Fix osadm manage-node --schedulable #10971
Conversation
|
[test] |
marun
force-pushed
the
fix-node-schedulable
branch
2 times, most recently
from
9e1bf2c
to
148de23
Compare
marun
force-pushed
the
fix-node-schedulable
branch
from
148de23
to
9187766
Compare
| @@ -71,7 +71,8 @@ status: | |||
| ' | oc create -f -" | |||
|
|
|||
| os::cmd::expect_success_and_text 'oadm manage-node --selector= --schedulable=true' 'Ready' | |||
| os::cmd::expect_success_and_not_text 'oadm manage-node --selector= --schedulable=true' 'Sched' | |||
| os::cmd::expect_success_and_not_text 'oadm manage-node --selector= --schedulable=true' 'SchedulingDisabled' | |||
| os::cmd::expect_success_and_text 'oadm manage-node --selector= --schedulable=false' 'SchedulingDisabled' | |||
There was a problem hiding this comment.
Add another "get" test after each of these that validates that the node is actually schedulable. I.e. expect_succes_and_text 'oc get node ... --template="{{ .status.conditions ... }}"
There was a problem hiding this comment.
We don't have a node in test/cmd - that tests needs to live in e2e or extended
There was a problem hiding this comment.
@smarterclayton There is already a kubelet unit test that validates the translation from Unschedulable: true to SchedulingDisabled. I don't think there is value in duplicating that coverage here.
|
LGTM |
|
@smarterclayton merge please |
|
You can create a node using the API in test-cmd pretty easily. The point is to test the command, not the kubelet. |
|
Be careful about nodes in test-cmd. I feel like at one point it made the scheduler crazy to have "Ready" nodes that weren't actually ready |
marun
force-pushed
the
fix-node-schedulable
branch
from
9187766
to
754dbd9
Compare
|
@smarterclayton afaict this fix is adequately tested. If extra testing of this functional area is desired, please consider filing an issue and having it implemented separately rather than delaying the unblocking of the networking tests any further. |
|
Evaluated for origin test up to 754dbd9 |
|
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9206/) |
|
LGTM [merge] |
|
[merge] |
|
#11016 again, [merge] |
|
Evaluated for origin merge up to 754dbd9 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9456/) (Image: devenv-rhel7_5096) |
Fixes #10970.
cc: @pravisankar