sa: make docker registry service controller check all secrets #19788
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfojtik 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 |
openshift-ci-robot
added
approved
| @@ -241,18 +241,14 @@ func getDockerRegistryLocations(lister listers.ServiceLister, location serviceLo | |||
| func (e *DockerRegistryServiceController) syncRegistryLocationChange(key string) error { | |||
| newLocations := e.getDockerRegistryLocations() | |||
| newDockerRegistryLocations := sets.NewString(newLocations...) | |||
| existingURLs := e.getRegistryURLs() | |||
| if existingURLs.Equal(newDockerRegistryLocations) { | |||
There was a problem hiding this comment.
we can limit the full scan to the "empty to something" transition, right?
There was a problem hiding this comment.
what do you mean? this is just checking if the docker-registry service moved I believe (the ClusterIP or name?)... we will still have to go secret by secret to decode the dockercfg and determine whether we have all endpoints we want there...
There was a problem hiding this comment.
what do you mean? this is just checking if the docker-registry service moved I believe (the ClusterIP or name?)... we will still have to go secret by secret to decode the dockercfg and determine whether we have all endpoints we want there...
The link you linked me to before. Like this: #19791
|
@deads2k need more thoughts to make the unit test pass... it seems like when you remove the registry service the secret is still present where it should be cleared up, will keep digging tomorrow. |
mfojtik
force-pushed
the
sa-10-fix-docker-registry-secrets
branch
from
b94c3bd to
a0255f5
Compare
openshift-ci-robot
added
size/M
|
@deads2k i picked yours (#19791) but after a while I realized that your change is making the unit tests somehow racy... Alternative approach... Lets not hide the intent to run an initial sweep of secrets when the controller starts and have a special variable to indicate that instead of using the registry urls vars (which I guess is also used by other controllers...). This way we don't need to change the unit tests, actually I can write a new one to exercise the initial sweep. WDYT? |
| } | ||
|
|
||
| t.Logf("deleting %s service", registryService.Name) |
There was a problem hiding this comment.
will keep these as they proved to be helpful when debugging
mfojtik
force-pushed
the
sa-10-fix-docker-registry-secrets
branch
from
a0255f5 to
bba8265
Compare
mfojtik
force-pushed
the
sa-10-fix-docker-registry-secrets
branch
from
bba8265 to
d7738c7
Compare
|
/retest |
mfojtik
force-pushed
the
sa-10-fix-docker-registry-secrets
branch
2 times, most recently
from
010c619 to
590bb9a
Compare
|
/retest |
1 similar comment
|
/retest |
| newLocations := e.getDockerRegistryLocations() | ||
| newDockerRegistryLocations := sets.NewString(newLocations...) | ||
| existingURLs := e.getRegistryURLs() | ||
| if existingURLs.Equal(newDockerRegistryLocations) { | ||
| glog.V(4).Infof("No effective update: %v", newDockerRegistryLocations) |
There was a problem hiding this comment.
nit: seems like you may want to keep this
| @@ -229,8 +254,6 @@ func TestUpdateOldStyleSecretWithKey(t *testing.T) { | |||
| t.Fatalf("failed to become ready") | |||
| } | |||
|
|
|||
| fakeWatch.Modify(registryService) | |||
There was a problem hiding this comment.
this was because the test previous had to kick something and doesn't need to anymore?
There was a problem hiding this comment.
yes, proving the initial secret check should work regardless of the registry change
|
optional nit, one question, lgtm. Feel free to tag with or without the nit. |
mfojtik
force-pushed
the
sa-10-fix-docker-registry-secrets
branch
from
590bb9a to
99e40f3
Compare
This change will make the docker registry service controller check all secrets regardless
whether the docker registry service location changed or not. This should effectively fix
all secrets in the cluster that are missing the registry DNS in the dockercfg content.
The drawback here is that the controller will now need to decode all dockercfg secrets
to perform the check, which might increase the load when the controller starts.
However, the controller resync is zero which means it won't run very often, so I think the CPU time here is worth to spend to make sure the dockercfg content in the secret is correct ;-)
Should fix https://bugzilla.redhat.com/show_bug.cgi?id=1578195
/cc @soltysh
/cc @deads2k