Skip to content

Update the container init script to not use the legacy workaround#88

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
JoelSpeed:update-exec-script
Jul 13, 2021
Merged

Update the container init script to not use the legacy workaround#88
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
JoelSpeed:update-exec-script

Conversation

@JoelSpeed
Copy link
Copy Markdown
Contributor

This updates the init script that must be present on pods/containers to remove the legacy fallback. I've also added a note to the test that links to where the current solution was implemented in MCO. ie openshift/machine-config-operator#2232

@openshift-ci openshift-ci Bot requested review from Danil-Grigorev and mandre July 9, 2021 10:24
Comment thread pkg/cloud/cloud_test.go
func checkContainerCommand(t *testing.T, podSpec corev1.PodSpec) {
binBash := "/bin/bash"
dashC := "-c"
// This script should be present on every node.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// This script should be present on every node.
// This script should be present on every master node.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I wonder if it matters that masters and workers will communicate with API Server using different load balancers. Should it be documented somewhere? That if worker is running CNM replica (Azure) then it is connected to a different endpoint? @JoelSpeed It is mostly an implementational detail, but still.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We will need this to be every node for Azure, so we are fixing that on the MCO side already. This comment will be updated to reflect the changes and the differences once that is merged

@Danil-Grigorev
Copy link
Copy Markdown

/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 9, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2021
@Danil-Grigorev Danil-Grigorev mentioned this pull request Jul 9, 2021
@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2021
@JoelSpeed JoelSpeed force-pushed the update-exec-script branch from e528be8 to b43eca5 Compare July 12, 2021 14:36
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2021
@lobziik
Copy link
Copy Markdown
Contributor

lobziik commented Jul 13, 2021

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021
@openshift-bot
Copy link
Copy Markdown

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Copy Markdown

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown

/retest

Please review the full test history for this PR and help us cut down flakes.

@JoelSpeed
Copy link
Copy Markdown
Contributor Author

/override ci/prow/e2e-aws-upgrade

This PR serves no functional changes as we never observed the value we were setting here anyway, no real need for the upgrade to pass as this won't affect upgrades

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 13, 2021

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/e2e-aws-upgrade

Details

In response to this:

/override ci/prow/e2e-aws-upgrade

This PR serves no functional changes as we never observed the value we were setting here anyway, no real need for the upgrade to pass as this won't affect upgrades

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lobziik
Copy link
Copy Markdown
Contributor

lobziik commented Jul 13, 2021

/override ci/prow/e2e-aws-upgrade

We didn't ever handle this variable, shouldn't have any effect at all.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Jul 13, 2021

@lobziik: Overrode contexts on behalf of lobziik: ci/prow/e2e-aws-upgrade

Details

In response to this:

/override ci/prow/e2e-aws-upgrade

We didn't ever handle this variable, shouldn't have any effect at all.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit 5569d72 into openshift:master Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants