pkg/daemon: remove annotation equality test from validation #138
Conversation
openshift-ci-robot
added
approved
|
/test e2e-aws |
|
/lgtm |
| // system state is valid. | ||
| if strings.Compare(dcAnnotation, ccAnnotation) == 0 { | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
This makes sense to me. Though... now I wonder about the opposite case. If the annotations do match, but isDesiredMachineState is false, then something has gone wrong, right? I guess this gets into #99, but it's slightly different: there, we're talking about repeatedly trying and failing to meet the desired config so that we can set the current config label to the same. Here, we've already in the past validated the node for that config (since the current config label already matches), but somehow the state doesn't match. I feel like we should just set it to degraded in that case?
So basically, it'd mean still doing this comparison but e.g. in process after the call to isDesiredMachineState(). Anyway, we can do that in a follow-up!
There was a problem hiding this comment.
my thought was that if there was a one-time change of a file under our control, that we can (and I think should) easily just change it back to what we want. if it happens multiple times right now, we don't do anything, but that was already true (maybe there are more cases of it now). I think doing some kind of backoff like described in #99 is the way to go in the long run, which I think would cover this case as well.
pkg/daemon/daemon.go
Outdated
| "path/filepath" | ||
| "strings" | ||
| "os" |
There was a problem hiding this comment.
That's probably from me not getting in the habit yet of running gofmt. :)
|
/lgtm |
|
I don't think the e2e test is going to return. It's been "building" for 4 hours and the Jenkins link 404s. |
|
I see a page when I click on the details link - it says it's not finished, started at 7:30ish PDT and doesn't have any build logs. seems busted. will it restart if I tell it to retest? |
|
let's try it /retest e2e-aws |
|
well that didn't seem to restart it @smarterclayton any ideas? |
|
@sdemos the bot is currently down and being fixed. |
|
/retest e2e-aws |
|
/lgtm (to retrigger based on @jlebon's command) |
|
/test e2e-aws |
|
/retest |
sdemos
force-pushed
the
always-validate
branch
from
c66af16
to
a33d829
Compare
|
/test e2e-aws |
1 similar comment
|
/test e2e-aws |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
Heh, OK this is failing on: Which took me a while to understand, but I think it's due to the merge conflicts. Can you rebase this? |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
rebase 😄 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
4 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Specifically: pkg/daemon/daemon.go:285:1: syntax error: unexpected <<, expecting } |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
the node state validation function had a test right at the beginning that checked if the values of the current annotation and the desired annotation were equal. if they were, it returned, under the assumption that because the annotations were equal the node was in the state we wanted it to be in. with the way that we try to behave, in general that would be true. however, we instead opt for more rigorous testing of the node - every time the validation function is called, it does a full validation of the node state. if something changed, we attempt to reconcile the state back to the desired. this also reflects the fact that nobody should be modifying the node outside of the daemon, and if they do they should expect to get clobbered. also, my editor ran gofmt on the file and fixed the import ordering. fixes openshift#98
sdemos
force-pushed
the
always-validate
branch
from
a33d829
to
df97fcf
Compare
|
alright, it's rebased. let's see if the tests pass. |
|
/lgtm ... while we can! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ashcrow, jlebon, sdemos The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
remove extra get on crd during imagestream processing (a conflict win…
the node state validation function had a test right at the beginning
that checked if the values of the current annotation and the desired
annotation were equal. if they were, it returned, under the assumption
that because the annotations were equal the node was in the state we
wanted it to be in.
with the way that we try to behave, in general that would be true.
however, we instead opt for more rigorous testing of the node - every
time the validation function is called, it does a full validation of the
node state. if something changed, we attempt to reconcile the state back
to the desired. this also reflects the fact that nobody should be
modifying the node outside of the daemon, and if they do they should
expect to get clobbered.
also, my editor ran gofmt on the file and fixed the import ordering.
fixes #98
/cc @ashcrow @jlebon