-
Notifications
You must be signed in to change notification settings - Fork 39.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Federated ConfigMap controller #35635
Conversation
Jenkins GCE etcd3 e2e failed for commit 8c4ba03. Full PR test history. The magic incantation to run this job again is |
Jenkins GCI GCE e2e failed for commit 8c4ba03. Full PR test history. The magic incantation to run this job again is |
Jenkins GCE e2e failed for commit 8c4ba03. Full PR test history. The magic incantation to run this job again is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass review complete. LMK when you're ready for a second pass.
configmapcontroller.configmapDeliverer = util.NewDelayingDeliverer() | ||
configmapcontroller.clusterDeliverer = util.NewDelayingDeliverer() | ||
|
||
// Start informer in federated API servers on configmaps that should be federated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/in/on ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}, | ||
) | ||
|
||
// Federated updeater along with Create/Update/Delete operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: updater
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
util.StartBackoffGC(configmapcontroller.configmapBackoff, stopChan) | ||
} | ||
|
||
func getConfigMapKey(namespace, name string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per previous reviews, perhaps just use NamespacedName.String() to avoid redundancy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, need to fix in other controllers
} | ||
|
||
// Internal structure for data in delaying deliverer. | ||
type configmapItem struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use NamespacedName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
func (configmapcontroller *ConfigMapController) deliverConfigMapObj(obj interface{}, delay time.Duration, failed bool) { | ||
configmap := obj.(*api_v1.ConfigMap) | ||
configmapcontroller.deliverConfigMap(configmap.Namespace, configmap.Name, delay, failed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use NamespacedName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
}) | ||
|
||
if err != nil { | ||
glog.Errorf("Failed to execute updates for %s: %v", key, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Mention retrying in ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
return | ||
} | ||
|
||
// Evertyhing is in order but lets be double sure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per previous review. No. Either retry with a good reason, or don't retry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
assert.Equal(t, configmap1.Namespace, updatedConfigMap.Namespace) | ||
assert.True(t, configmapsEqual(configmap1, *updatedConfigMap2)) | ||
|
||
// Test add cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Probably good to add a test for a cluster that exists but is offline when the update is performed. Check that it is not updated while offline (to test that the test is actually working), and that it does update once the cluster comes online again.
close(stop) | ||
} | ||
|
||
func configmapsEqual(a, b api_v1.ConfigMap) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not right. There are other fields that can be different (e.g. UID). Why not use the Equivalent functions in the library, which do a proper job of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// the future the ConfigMap structure is expanded then any field that is not populated. | ||
// by the api server should be included here. | ||
func ConfigMapEquivalent(s1, s2 api_v1.ConfigMap) bool { | ||
return ObjectMetaEquivalent(s1.ObjectMeta, s2.ObjectMeta) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the specs being equal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no spec in ConfigMap :)
8c4ba03
to
f827c0f
Compare
Jenkins verification failed for commit f827c0fa321c7974a53597922be35b2ed813edbb. Full PR test history. The magic incantation to run this job again is |
Jenkins GKE smoke e2e failed for commit f827c0fa321c7974a53597922be35b2ed813edbb. Full PR test history. The magic incantation to run this job again is |
Jenkins GCI GKE smoke e2e failed for commit f827c0fa321c7974a53597922be35b2ed813edbb. Full PR test history. The magic incantation to run this job again is |
f827c0f
to
d010d1d
Compare
LGTM. Thanks @mwielgus |
@mwielgus |
@mwielgus |
@quinton-hoole that was already fixed. verification is green. |
Automatic merge from submit-queue |
Based on the secrets controller. E2e tests will come in the next PR.
Release note:
cc: @quinton-hoole @kubernetes/sig-cluster-federation
This change is![Reviewable](https://camo.githubusercontent.com/2d899f4291d07d3cd2fa4aaae1e3b243f164c23fce87d30a589ace0d496a444c/68747470733a2f2f72657669657761626c652e6b756265726e657465732e696f2f7265766965775f627574746f6e2e737667)