Ensure client can delete a buildconfig with no associated builds, eve… #9657
Conversation
|
@jupierce if you put "fixes [issue link]" in your PR summary, github will auto-close the issue when the PR merges. |
|
(there are some other syntaxes that work, but i think think "issue XXX" is one of them) |
| builds, err = reaper.oc.Builds(namespace).List(kapi.ListOptions{LabelSelector: buildutil.BuildConfigSelector(name)}) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
you also need to check the deprecated label.
There was a problem hiding this comment.
and, due to the truncation issue, you need to filter the results using the annotation value, like the rest of the code does, to determine if a build is really related to the buildconfig.
|
There is a bigger issue in this code that i'd like @csrwng to weigh in on... now that we are truncating the buildconfig label value, we can't just do straight selection using the buildconfig name, we also need to truncate the buildconfig name first, then filter the selection results using the annotation value. @csrwng i thought you updated all uses of the buildconfig label(s), did this just get missed or is there a reason this didn't need to change that i'm not seeing? It looks like the code was updated (since it uses the annotation) but it's still doing the label selection using the full name. Does the label selection process itself truncate the value? |
|
might be worth adding a test as well that ensures we delete only the correct set of builds when someone has created two buildconfigs with very long names, a set of builds associated with each, and then deletes one of those buildconfigs. |
| // Add deletion pending annotation to the build config | ||
| err := unversioned.RetryOnConflict(unversioned.DefaultRetry, func() error { | ||
| bc, err := reaper.oc.BuildConfigs(namespace).Get(name) | ||
| if kerrors.IsNotFound(err) { |
There was a problem hiding this comment.
I could not determine a reason for the special handling of not found. Any non-conflict error returned to RetryOnConflict appears to terminate retries immediately. The only effect of manually creating a NewNotFound seemed to be removing the "s" from the ultimate error message:
Error from server: buildconfigs "test" not found
There was a problem hiding this comment.
i think there was a point in time at which we would still attempt to delete builds that were associated with the buildconfig, even if we couldn't find the buildconfig.
that behavior was recently changed such that we just error out if we can't find the buildconfig and do nothing further. So yeah, this logic is probably no longer needed.
|
@bparees Looks like the build config selector is truncated automatically during its instantiation. |
|
The submitted implementation does not attempt to address the race condition between deletion of a build configuration by client A and triggering of a new build by a competing client B. Because of etcd-io/etcd#741 and the fact that OpenShift does not use quorum reads, we would need to redesign the buildconfig deletion process to use a controller/reconciliation loop to ensure deterministic cascading deletions. If we are OK with the race window existing, we could requery associated builds after pausing the build config to reduce the window further (with a slight performance impact). |
|
lgtm, can you add the test for long buildconfig names? |
|
(i'm ok w/ the race window, given that we can't eliminate it completely i don't know if it's worth jumping through hoops to make it a little smaller) |
|
@bparees Added test case for your perusal. |
| } | ||
| } | ||
| }, | ||
| "output": { |
There was a problem hiding this comment.
you can make this buildconfig execute faster by leaving out the output so it doesn't have to push the resulting image anywhere.
|
[test] |
| expected: []ktestclient.Action{ | ||
| ktestclient.NewGetAction("buildconfigs", "default", configName), | ||
| ktestclient.NewUpdateAction("buildconfigs", "default", makeBuildConfig(0, true)), | ||
| // Since tehre are no builds associated with this build config, do not expect an update |
There was a problem hiding this comment.
sigh.. I'm coming from an IDE with spellcheck.
|
[testextended][extended:core(extremely long build/bc names are not problematic)] |
…n if user lacks build strategy permission
|
Evaluated for origin test up to 22d2378 |
|
Evaluated for origin testextended up to 22d2378 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5898/) |
|
@jupierce i deleted your builds and images test tags so we can see if we can get a clean run just on your new extended test. |
|
continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/297/) (Extended Tests: core(extremely long build/bc names are not problematic)) |
|
lgtm |
|
known flake #9681 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5937/) (Image: devenv-rhel7_4548) |
|
flake #9444 |
|
Evaluated for origin merge up to 22d2378 |
@bparees Fixes #8655