OCPNODE-2763: extended: add minimum kubelet version test #29353
Conversation
haircommander
force-pushed
the
min-kubelet-version
branch
2 times, most recently
from
f385e00 to
d84b1e7
Compare
|
/test |
|
@haircommander: The The following commands are available to trigger optional jobs: Use In response to this:
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-sigs/prow repository. |
|
/test e2e-gcp-ovn-techpreview-serial |
|
/test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview-serial |
|
Job Failure Risk Analysis for sha: d84b1e7
|
haircommander
force-pushed
the
min-kubelet-version
branch
from
d84b1e7 to
b370f00
Compare
openshift-merge-robot
added
the
needs-rebase
|
Job Failure Risk Analysis for sha: b370f00
|
haircommander
force-pushed
the
min-kubelet-version
branch
from
b370f00 to
093b502
Compare
openshift-merge-robot
removed
the
needs-rebase
|
Job Failure Risk Analysis for sha: 093b502
|
haircommander
force-pushed
the
min-kubelet-version
branch
from
093b502 to
da2df6a
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haircommander The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/retest |
|
/test e2e-gcp-ovn-techpreview |
|
Can you satisfy the jira gods? either by linking with a jira ticket or adding no-jira. |
|
Code looks good to me! But the jobs seem unhappy. |
haircommander
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@haircommander: This pull request references OCPNODE-2763 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
|
|
||
| if nodesConfigOrig.Spec.MinimumKubeletVersion == version { | ||
| return nil |
There was a problem hiding this comment.
nit, could return an empty function here and skip the nil checks in the callers.
There was a problem hiding this comment.
not short circuting here means we still call waitForAPIServerRollout and do a bunch of needless waiting, so we need to branch in some way here to save time
edit: corrected the function we call
|
/retest |
|
/skip |
|
Job Failure Risk Analysis for sha: da2df6a
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: da2df6a
New tests seen in this PR at sha: da2df6a
|
haircommander
force-pushed
the
min-kubelet-version
branch
from
da2df6a to
bfb266c
Compare
|
Job Failure Risk Analysis for sha: bfb266c
|
| desiredTestDuration = 25 * time.Minute | ||
| ) | ||
|
|
||
| var _ = g.Describe("[sig-node][OCPFeatureGate:MinimumKubeletVersion] ", func() { |
There was a problem hiding this comment.
shouldn't this test be marked as Serial ?
There was a problem hiding this comment.
I haven't seem them step on each other in a way that implies that but I'm open to it if you think it's better.
There was a problem hiding this comment.
| oc := exutil.NewCLIWithoutNamespace("minimum-kubelet-version") | ||
|
|
||
| g.DescribeTable("admission", func(version string, expectedErr bool) { | ||
| defer updateMinimumKubeletVersion(oc, version, expectedErr) |
There was a problem hiding this comment.
shouldn't updateMinimumKubeletVersion wait for kas rollout ?
There was a problem hiding this comment.
for these admission tests we don't need the rollout as we're just checking whether we can change the value or not. Below we pair the update with a wait to make sure we deny the node access if appropriate
| }, | ||
| g.Entry("should allow an empty minimum kubelet version", "", false), | ||
| g.Entry("should allow an old minimum kubelet version", "1.30.0", false), | ||
| g.Entry("should not allow with a new minimum kubelet version", "1.100.0", true), |
There was a problem hiding this comment.
worth checking the expected error from the admission plugin.
haircommander
force-pushed
the
min-kubelet-version
branch
from
bfb266c to
2c0aecb
Compare
|
/test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview-serial |
|
Job Failure Risk Analysis for sha: 2c0aecb
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 2c0aecb
New tests seen in this PR at sha: 2c0aecb
|
| defer updateMinimumKubeletVersion(oc, version, handleErrFunc) | ||
| }, | ||
| g.Entry("should allow an empty minimum kubelet version", "", expectNoError), | ||
| g.Entry("should allow an old minimum kubelet version", "1.30.0", expectNoError), |
There was a problem hiding this comment.
Maybe I don't understand something, but I think this case will set a new minimum version, which will cause a new revision.
In that case, I think we should ensure that a parallel test (if this test runs in parallel with others) won't observe a "partial state". I think that we should make sure the servers are stable.
There was a problem hiding this comment.
good point. I have replaced those two with one other failing test. both 1.30.0 and "" will be set in the authorization test anyway, so I think this is better. now the failure cases will deny and can be run in parallel
haircommander
force-pushed
the
min-kubelet-version
branch
from
2c0aecb to
28727f3
Compare
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 28727f3
New tests seen in this PR at sha: 28727f3
|
|
@haircommander are we merging this tests ? |
Signed-off-by: Peter Hunt <pehunt@redhat.com>
haircommander
force-pushed
the
min-kubelet-version
branch
from
28727f3 to
51b3f1d
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
@haircommander: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
Job Failure Risk Analysis for sha: 51b3f1d
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 51b3f1d
New tests seen in this PR at sha: 51b3f1d
|
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
|
@haircommander is this still relevant? |
requires openshift/kubernetes#2104 and openshift/cluster-kube-apiserver-operator#1754