Disable daemon set deletion behaviour #754
Conversation
pkg/ds/daemon_set_test.go
Outdated
|
|
||
| err = waitForPodsInIntent(consulStore, 0) | ||
| err = waitForPodsInIntent(consulStore, 9) |
There was a problem hiding this comment.
tell me about this 9... it's certainly not 11, which makes me wonder about it
There was a problem hiding this comment.
I think it should be 23
pkg/ds/farm_test.go
Outdated
| @@ -512,7 +512,8 @@ func TestFarmSchedule(t *testing.T) { | |||
| Assert(t).IsNil(err, "Expected daemon set to be deleted in farm") | |||
|
|
|||
| // Verify node3 is unscheduled | |||
| labeled, err = waitForPodLabel(applicator, false, "node3/testPod") | |||
| // Behaviour change! Daemon Set deletions do not delete their pods (for now) | |||
| labeled, err = waitForPodLabel(applicator, true, "node3/testPod") | |||
| Assert(t).IsNil(err, "Expected pod not to have a dsID label") | |||
The implementation that unscheduled all pods was hard to build a failsafe around, so we prefer to remove it altogether until we can get a proper failsafe built.
Schedule, to me, means /intent not the label tree
rudle
force-pushed
the
disable-ds-delete
branch
from
b143696
to
b9afecb
Compare
|
tests pass! |
pkg/ds/daemon_set_test.go
Outdated
| @@ -306,13 +307,14 @@ func TestSchedule(t *testing.T) { | |||
| if err != nil { | |||
| t.Fatalf("Unable to delete daemon set: %v", err) | |||
| } | |||
| numNodes = waitForNodes(t, ds, 0, desiresErrCh, dsChangesErrCh) | |||
| Assert(t).AreEqual(numNodes, 0, "took too long to unschedule") | |||
| // Behaivour change: Deleting a daemon set will no longer unschedule its pods (for now) | |||
There was a problem hiding this comment.
you spelled it wrong, and I'm not talking about the ou, I'm talking about the iv
pkg/ds/daemon_set_test.go
Outdated
| @@ -302,20 +302,22 @@ func TestSchedule(t *testing.T) { | |||
| // | |||
| // Deleting the daemon set should unschedule all of its nodes | |||
| // | |||
| // beforeDeletePods := labeledPods(t, ds) | |||
There was a problem hiding this comment.
why leave this commented line in?
edit: oh hmm it gets made into a real line in next commit. oooook
pkg/ds/daemon_set_test.go
Outdated
| ds.logger.NoFields().Info("Deleting daemon set...") | ||
| err = dsStore.Delete(ds.ID()) | ||
| if err != nil { | ||
| t.Fatalf("Unable to delete daemon set: %v", err) | ||
| } | ||
| // Behaivour change: Deleting a daemon set will no longer unschedule its pods (for now) | ||
| numNodes = waitForNodes(t, ds, expectedNodes, desiresErrCh, dsChangesErrCh) | ||
| Assert(t).AreEqual(numNodes, expectedNodes, "Unexpected number of nodes scheduled") | ||
| numNodes = waitForNodes(t, ds, 12, desiresErrCh, dsChangesErrCh) |
There was a problem hiding this comment.
expected->12 in previous commit might be good, cuz this ain't schedule -> label
There was a problem hiding this comment.
yeah, this number is nonsense - this test will fail. 🤷♂️
| @@ -506,7 +506,7 @@ func TestFarmSchedule(t *testing.T) { | |||
| dsID = labeled.Labels.Get(DSIDLabel) | |||
| Assert(t).AreEqual(anotherDSData.ID.String(), dsID, "Unexpected dsID labeled") | |||
|
|
|||
| dsStore.Delete(anotherDSData.ID) | |||
| err = dsStore.Delete(anotherDSData.ID) | |||
There was a problem hiding this comment.
that was a line I added?
yeah it was.
I am sorry. The errors are completely bogus because I didn't assign too...
|
Merging on green |
|
|
||
| err = waitForPodsInIntent(consulStore, 0) | ||
| Assert(t).IsNil(err, "Unexpected number of pods scheduled") | ||
| err = waitForPodsInIntent(consulStore, len(beforeDeletePods)) |
There was a problem hiding this comment.
maybe this is what's causing test flakes - we'd like it to be 12 (Right?), but sometimes tests expect it to be 1, presumably because the scheduledPods call doesn't get all of them.
| numNodes = waitForNodes(t, ds, 0, desiresErrCh, dsChangesErrCh) | ||
| Assert(t).AreEqual(numNodes, 0, "took too long to unschedule") | ||
| // behavior change: Deleting a daemon set will no longer unschedule its pods (for now) | ||
| numNodes = waitForNodes(t, ds, 12, desiresErrCh, dsChangesErrCh) |
There was a problem hiding this comment.
hey hold on a second. This line waits for the number of nodes to decrease to 12 (spoiler, it never will)
There was a problem hiding this comment.
This doesn't cause any test failures of course, but it adds 10 seconds to each test.
Then again, you may say that's a feature: We make sure that no pods are deleted for ten seconds.
| Assert(t).AreEqual(numNodes, 0, "took too long to unschedule") | ||
| // behavior change: Deleting a daemon set will no longer unschedule its pods (for now) | ||
| numNodes = waitForNodes(t, ds, 12, desiresErrCh, dsChangesErrCh) | ||
| Assert(t).AreEqual(numNodes, expectedNodes, "Unexpected number of nodes labeled") |
There was a problem hiding this comment.
and then this line checks that the number of nodes is 23, not 12
The implementation that unscheduled all pods was hard to build a failsafe
around, so we prefer to remove it altogether until we can get a proper failsafe
built.