Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated mode: http2 support #4004

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Updated mode: http2 support #4004

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f813b9
Updated mode: http2 support
ankit864 Jan 10, 2025
f360561
update URL scheme handling in case of http2 mode.
ankit864 Jan 20, 2025
106753b
Add http2 handling for healthcheck mode
ankit864 Jan 20, 2025
c06a2e9
condition for http is not required.
ankit864 Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion paasta_tools/cli/cmds/info.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
)

# modes that depend on smartstack port cannot be tested via paasta proxies, so we exclude those
TESTABLE_SERVICE_MODES = {"http", "https"}
TESTABLE_SERVICE_MODES = {"http", "https", "http2"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_smartstack_endpoints() will probably need some special handling since that passes through the mode as the uri scheme - but the http2 scheme is still http

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and i think the same needs to happen in get_deployments_strings()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handling this with url_scheme even though service is http2 we will use http in endpoint url. Fixed in both cases.



def add_subparser(subparsers):
Expand Down
2 changes: 1 addition & 1 deletion paasta_tools/cli/cmds/local_run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ def run_healthcheck_on_container(
healthcheck_result = perform_cmd_healthcheck(
docker_client, container_id, healthcheck_data, timeout
)
elif healthcheck_mode == "http" or healthcheck_mode == "https":
elif healthcheck_mode in ["http", "https", "http2"]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-blocking suggestion: might be nice to have a constant for HTTP_MODES = {"http", "https", "http2"} since we have this list in multiple places

healthcheck_result = perform_http_healthcheck(healthcheck_data, timeout)
elif healthcheck_mode == "tcp":
healthcheck_result = perform_tcp_healthcheck(healthcheck_data, timeout)
Expand Down
2 changes: 2 additions & 0 deletions paasta_tools/cli/schemas/smartstack_schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
"enum": [
"http",
"https",
"http2",
"tcp"
]
},
Expand Down Expand Up @@ -117,6 +118,7 @@
"enum": [
"http",
"https",
"http2",
"tcp"
]
},
Expand Down
2 changes: 1 addition & 1 deletion paasta_tools/kubernetes_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1371,7 +1371,7 @@ def get_liveness_probe(
timeout_seconds=timeout_seconds,
)

if mode == "http" or mode == "https":
if mode in ["http", "https", "http2"]:
path = self.get_healthcheck_uri(service_namespace_config)
probe.http_get = V1HTTPGetAction(
path=path, port=self.get_container_port(), scheme=mode.upper()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode.upper() will lead to LivenessProbe w/ scheme=HTTP which isn't supported , so we need to do some conversion here

Copy link
Contributor Author

@ankit864 ankit864 Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am handling this in get_healthcheck_mode func as most of the places we are relying on right conversion of healthcheck_mode so fixing that and making http as default value for http2 service if healthcheck_mode is not defined explicitly will take care of this case.

Expand Down
4 changes: 2 additions & 2 deletions paasta_tools/long_running_service_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def get_mode(self) -> str:
return None
else:
return "http"
elif mode in ["http", "tcp", "https"]:
elif mode in ["http", "http2", "tcp", "https"]:
return mode
else:
raise InvalidSmartstackMode("Unknown mode: %s" % mode)
Expand Down Expand Up @@ -424,7 +424,7 @@ def get_healthcheck_for_instance(
mode = service_manifest.get_healthcheck_mode(smartstack_config)
hostname = socket.getfqdn()

if mode == "http" or mode == "https":
if mode in ["http", "https", "http2"]:
path = service_manifest.get_healthcheck_uri(smartstack_config)
healthcheck_command = "%s://%s:%d%s" % (mode, hostname, random_port, path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(same as above re: http2 having a uri scheme of http)

elif mode == "tcp":
Expand Down