K8s: refactor local connection server usage#927
Open
wanyunSu wants to merge 6 commits into
Open
Conversation
| @@ -2160,7 +2210,7 @@ def _wait_for_lcs_readiness(self, podname: str, session: str) -> None: | |||
|
|
|||
| self._wait_for_nodeport_http_ready(url, remaining_time) | |||
Collaborator
There was a problem hiding this comment.
Question here - why is it that you run the check for nodeport readiness after you call _wait_for_pod_api_ready?
Contributor
Author
There was a problem hiding this comment.
_wait_for_pod_api_ready returns node_name , which is required to construct the NodePort url, also, the NodePort can't be reachable before the pod is running
| # --- STAGE 2: Wait for NodePort to be externally reachable (using HTTP urllib) --- | ||
| url = f"http://{node_name}:{self.connection_server_node_port}" | ||
| lcs = self._lcs_state_for(session) | ||
| url = f"http://{node_name}:{lcs.node_port}" |
Collaborator
There was a problem hiding this comment.
This would be cleaner if we had the url as an attribute of tthe _LcsSessionState
Contributor
Author
There was a problem hiding this comment.
There's no need to store url in _LcsSessionState, as it is only used as a readiness probe inside _wait_for_lcs_readiness and never read again after boot completes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes issue #825
Changes:
_LcsSessionStatedataclass replaces the old scalars for supporting multi-sessions. Sessions without an LCS have no entry._create_nodeport_servicetakes explicit port / node_port args, stops the root-controller branch from polluting the LCS dict._lcs_state.pop(session)on namespace deletion;lcs.is_booted = Falsein notify_termination when the LCS pod itself terminates.Type of change
List of required branches from other repositories
WHAT PRs NEED TO BE INCLUDED TO MAKE THE CHANGE.
Change log
see above
Suggested manual testing checklist
drunc-unified-shell k8s config/daqsystemtest/example-configs.data.xml local-1x1-config claudia-test
Developer checklist
Prior to marking this as "Ready for Review"
Tests ran on: np04-019 from NFD_DEV_260518_A9
Unit tests - some tests can't be ran on the CI. This is documented. If this PR checks a feature that can't be tested with CI, this has been marked appropriately.
Integration tests - the
daqsystemtest_integtest_bundlerequires a lot of resources, and connections to the EHN1 infrastructure. Check the cross referenced list if you can't run these. The developer needs to run at least the .pytest --marker) passeddaqsystemtest_integtest_bundle.sh -k minimal_system_quick_test.pydaqsystemtest_integtest_bundle.sh./scripts/drunc_integtest_bundle.sh)Final checklist prior to marking this as "Ready for Review"
Reviewer checklist
src/daqsystemtest_integtest_bundle.sh -k minimal_system_quick_test.pyifdaqsystemtest_integtest_bundle.shdruncare in the log filesdruncfailure appears:scripts/drunc_integtest_bundle.sh)Once the above boxes are checked, the PR(s) can be merged following the steps below.
Choose one of the following an complete all substepsPrior to merging
Once completed, the reviewer can merge the PR.
Notification message for a Slack channel
Note - this should be to #dunedaq-integration for general workflow that isn't during a release candidate period, and to #daq-release-prep otherwise.
For an single merge that changes the user workflow
For co-ordinated merge