From dd74a397009d35c3f9a49f44b6eb0cf3fa53395e Mon Sep 17 00:00:00 2001 From: Michael Taufen Date: Thu, 21 Dec 2017 09:27:30 -0800 Subject: [PATCH] Make ConfigOK status messages more human readable by including the API path to the object instead of the UID --- .../kubeletconfig/checkpoint/download.go | 4 +-- .../kubeletconfig/checkpoint/download_test.go | 4 +-- pkg/kubelet/kubeletconfig/configsync.go | 6 ++-- pkg/kubelet/kubeletconfig/controller.go | 16 +++++----- pkg/kubelet/kubeletconfig/status/status.go | 30 +++++++++---------- test/e2e_node/dynamic_kubelet_config_test.go | 25 +++++++++------- 6 files changed, 45 insertions(+), 40 deletions(-) diff --git a/pkg/kubelet/kubeletconfig/checkpoint/download.go b/pkg/kubelet/kubeletconfig/checkpoint/download.go index 9778f318b7..9b516a0eb7 100644 --- a/pkg/kubelet/kubeletconfig/checkpoint/download.go +++ b/pkg/kubelet/kubeletconfig/checkpoint/download.go @@ -128,13 +128,13 @@ func (r *remoteConfigMap) Download(client clientset.Interface) (Checkpoint, stri // get the ConfigMap via namespace/name, there doesn't seem to be a way to get it by UID cm, err := client.CoreV1().ConfigMaps(r.source.ConfigMapRef.Namespace).Get(r.source.ConfigMapRef.Name, metav1.GetOptions{}) if err != nil { - reason = fmt.Sprintf(status.FailSyncReasonDownloadFmt, r.source.ConfigMapRef.Name, r.source.ConfigMapRef.Namespace) + reason = fmt.Sprintf(status.FailSyncReasonDownloadFmt, r.APIPath()) return nil, reason, fmt.Errorf("%s, error: %v", reason, err) } // ensure that UID matches the UID on the reference, the ObjectReference must be unambiguous if r.source.ConfigMapRef.UID != cm.UID { - reason = fmt.Sprintf(status.FailSyncReasonUIDMismatchFmt, r.source.ConfigMapRef.UID, cm.UID) + reason = fmt.Sprintf(status.FailSyncReasonUIDMismatchFmt, r.source.ConfigMapRef.UID, r.APIPath(), cm.UID) return nil, reason, fmt.Errorf(reason) } diff --git a/pkg/kubelet/kubeletconfig/checkpoint/download_test.go b/pkg/kubelet/kubeletconfig/checkpoint/download_test.go index ccca6c3161..345319fd37 100644 --- a/pkg/kubelet/kubeletconfig/checkpoint/download_test.go +++ b/pkg/kubelet/kubeletconfig/checkpoint/download_test.go @@ -131,11 +131,11 @@ func TestRemoteConfigMapDownload(t *testing.T) { // object doesn't exist {"object doesn't exist", &remoteConfigMap{&apiv1.NodeConfigSource{ConfigMapRef: &apiv1.ObjectReference{Name: "bogus", Namespace: "namespace", UID: "bogus"}}}, - nil, "failed to download ConfigMap"}, + nil, "not found"}, // UID of downloaded object doesn't match UID of referent found via namespace/name {"UID is incorrect for namespace/name", &remoteConfigMap{&apiv1.NodeConfigSource{ConfigMapRef: &apiv1.ObjectReference{Name: "name", Namespace: "namespace", UID: "bogus"}}}, - nil, "does not match UID"}, + nil, "does not match"}, // successful download {"object exists and reference is correct", &remoteConfigMap{&apiv1.NodeConfigSource{ConfigMapRef: &apiv1.ObjectReference{Name: "name", Namespace: "namespace", UID: "uid"}}}, diff --git a/pkg/kubelet/kubeletconfig/configsync.go b/pkg/kubelet/kubeletconfig/configsync.go index ce6b7e38bc..b128e95b92 100644 --- a/pkg/kubelet/kubeletconfig/configsync.go +++ b/pkg/kubelet/kubeletconfig/configsync.go @@ -139,7 +139,7 @@ func (cc *Controller) checkpointConfigSource(client clientset.Interface, source // if the checkpoint already exists, skip downloading if ok, err := cc.checkpointStore.Exists(uid); err != nil { - reason := fmt.Sprintf(status.FailSyncReasonCheckpointExistenceFmt, uid) + reason := fmt.Sprintf(status.FailSyncReasonCheckpointExistenceFmt, source.APIPath(), uid) return reason, fmt.Errorf("%s, error: %v", reason, err) } else if ok { utillog.Infof("checkpoint already exists for object with UID %q, skipping download", uid) @@ -155,7 +155,7 @@ func (cc *Controller) checkpointConfigSource(client clientset.Interface, source // save err = cc.checkpointStore.Save(checkpoint) if err != nil { - reason := fmt.Sprintf(status.FailSyncReasonSaveCheckpointFmt, checkpoint.UID()) + reason := fmt.Sprintf(status.FailSyncReasonSaveCheckpointFmt, source.APIPath(), checkpoint.UID()) return reason, fmt.Errorf("%s, error: %v", reason, err) } @@ -170,7 +170,7 @@ func (cc *Controller) setCurrentConfig(source checkpoint.RemoteConfigSource) (bo if source == nil { return false, status.FailSyncReasonSetCurrentLocal, err } - return false, fmt.Sprintf(status.FailSyncReasonSetCurrentUIDFmt, source.UID()), err + return false, fmt.Sprintf(status.FailSyncReasonSetCurrentUIDFmt, source.APIPath(), source.UID()), err } return updated, "", nil } diff --git a/pkg/kubelet/kubeletconfig/controller.go b/pkg/kubelet/kubeletconfig/controller.go index 9b959b4c94..83569622b8 100644 --- a/pkg/kubelet/kubeletconfig/controller.go +++ b/pkg/kubelet/kubeletconfig/controller.go @@ -137,7 +137,7 @@ func (cc *Controller) Bootstrap() (*kubeletconfig.KubeletConfiguration, error) { if err == nil { // set the status to indicate we will use the assigned config if curSource != nil { - cc.configOK.Set(fmt.Sprintf(status.CurRemoteMessageFmt, curSource.UID()), reason, apiv1.ConditionTrue) + cc.configOK.Set(fmt.Sprintf(status.CurRemoteMessageFmt, curSource.APIPath()), reason, apiv1.ConditionTrue) } else { cc.configOK.Set(status.CurLocalMessage, reason, apiv1.ConditionTrue) } @@ -171,7 +171,7 @@ func (cc *Controller) Bootstrap() (*kubeletconfig.KubeletConfiguration, error) { // set the status to indicate that we had to roll back to the lkg for the reason reported when we tried to load the assigned config if lkgSource != nil { - cc.configOK.Set(fmt.Sprintf(status.LkgRemoteMessageFmt, lkgSource.UID()), reason, apiv1.ConditionFalse) + cc.configOK.Set(fmt.Sprintf(status.LkgRemoteMessageFmt, lkgSource.APIPath()), reason, apiv1.ConditionFalse) } else { cc.configOK.Set(status.LkgLocalMessage, reason, apiv1.ConditionFalse) } @@ -271,14 +271,14 @@ func (cc *Controller) loadAssignedConfig(local *kubeletconfig.KubeletConfigurati // load from checkpoint checkpoint, err := cc.checkpointStore.Load(curUID) if err != nil { - return nil, src, fmt.Sprintf(status.CurFailLoadReasonFmt, curUID), err + return nil, src, fmt.Sprintf(status.CurFailLoadReasonFmt, src.APIPath()), err } cur, err := checkpoint.Parse() if err != nil { - return nil, src, fmt.Sprintf(status.CurFailParseReasonFmt, curUID), err + return nil, src, fmt.Sprintf(status.CurFailParseReasonFmt, src.APIPath()), err } if err := validation.ValidateKubeletConfiguration(cur); err != nil { - return nil, src, fmt.Sprintf(status.CurFailValidateReasonFmt, curUID), err + return nil, src, fmt.Sprintf(status.CurFailValidateReasonFmt, src.APIPath()), err } return cur, src, status.CurRemoteOkayReason, nil } @@ -301,14 +301,14 @@ func (cc *Controller) loadLastKnownGoodConfig(local *kubeletconfig.KubeletConfig // load from checkpoint checkpoint, err := cc.checkpointStore.Load(lkgUID) if err != nil { - return nil, src, fmt.Errorf("%s, error: %v", fmt.Sprintf(status.LkgFailLoadReasonFmt, lkgUID), err) + return nil, src, fmt.Errorf("%s, error: %v", fmt.Sprintf(status.LkgFailLoadReasonFmt, src.APIPath()), err) } lkg, err := checkpoint.Parse() if err != nil { - return nil, src, fmt.Errorf("%s, error: %v", fmt.Sprintf(status.LkgFailParseReasonFmt, lkgUID), err) + return nil, src, fmt.Errorf("%s, error: %v", fmt.Sprintf(status.LkgFailParseReasonFmt, src.APIPath()), err) } if err := validation.ValidateKubeletConfiguration(lkg); err != nil { - return nil, src, fmt.Errorf("%s, error: %v", fmt.Sprintf(status.LkgFailValidateReasonFmt, lkgUID), err) + return nil, src, fmt.Errorf("%s, error: %v", fmt.Sprintf(status.LkgFailValidateReasonFmt, src.APIPath()), err) } return lkg, src, nil } diff --git a/pkg/kubelet/kubeletconfig/status/status.go b/pkg/kubelet/kubeletconfig/status/status.go index 94598cae6e..797f7bbcb5 100644 --- a/pkg/kubelet/kubeletconfig/status/status.go +++ b/pkg/kubelet/kubeletconfig/status/status.go @@ -40,14 +40,14 @@ const ( NotDynamicLocalReason = "dynamic config is currently disabled by omission of --dynamic-config-dir Kubelet flag" // CurLocalMessage indicates that the Kubelet is using its local config, which consists of defaults, flags, and/or local files - CurLocalMessage = "using current (local)" + CurLocalMessage = "using current: local" // LkgLocalMessage indicates that the Kubelet is using its local config, which consists of defaults, flags, and/or local files - LkgLocalMessage = "using last-known-good (local)" + LkgLocalMessage = "using last-known-good: local" // CurRemoteMessageFmt indicates the Kubelet is using its current config, which is from an API source - CurRemoteMessageFmt = "using current (UID: %q)" + CurRemoteMessageFmt = "using current: %s" // LkgRemoteMessageFmt indicates the Kubelet is using its last-known-good config, which is from an API source - LkgRemoteMessageFmt = "using last-known-good (UID: %q)" + LkgRemoteMessageFmt = "using last-known-good: %s" // CurLocalOkayReason indicates that the Kubelet is using its local config CurLocalOkayReason = "when the config source is nil, the Kubelet uses its local config" @@ -55,20 +55,20 @@ const ( CurRemoteOkayReason = "passing all checks" // CurFailLoadReasonFmt indicates that the Kubelet failed to load the current config checkpoint for an API source - CurFailLoadReasonFmt = "failed to load current (UID: %q)" + CurFailLoadReasonFmt = "failed to load current: %s" // CurFailParseReasonFmt indicates that the Kubelet failed to parse the current config checkpoint for an API source - CurFailParseReasonFmt = "failed to parse current (UID: %q)" + CurFailParseReasonFmt = "failed to parse current: %s" // CurFailValidateReasonFmt indicates that the Kubelet failed to validate the current config checkpoint for an API source - CurFailValidateReasonFmt = "failed to validate current (UID: %q)" + CurFailValidateReasonFmt = "failed to validate current: %s" // LkgFail*ReasonFmt reasons are currently used to print errors in the Kubelet log, but do not appear in Node.Status.Conditions // LkgFailLoadReasonFmt indicates that the Kubelet failed to load the last-known-good config checkpoint for an API source - LkgFailLoadReasonFmt = "failed to load last-known-good (UID: %q)" + LkgFailLoadReasonFmt = "failed to load last-known-good: %s" // LkgFailParseReasonFmt indicates that the Kubelet failed to parse the last-known-good config checkpoint for an API source - LkgFailParseReasonFmt = "failed to parse last-known-good (UID: %q)" + LkgFailParseReasonFmt = "failed to parse last-known-good: %s" // LkgFailValidateReasonFmt indicates that the Kubelet failed to validate the last-known-good config checkpoint for an API source - LkgFailValidateReasonFmt = "failed to validate last-known-good (UID: %q)" + LkgFailValidateReasonFmt = "failed to validate last-known-good: %s" // FailSyncReasonFmt is used when the system couldn't sync the config, due to a malformed Node.Spec.ConfigSource, a download failure, etc. FailSyncReasonFmt = "failed to sync, reason: %s" @@ -78,21 +78,21 @@ const ( FailSyncReasonPartialObjectReference = "invalid ObjectReference, all of UID, Name, and Namespace must be specified" // FailSyncReasonUIDMismatchFmt is used when there is a UID mismatch between the referenced and downloaded ConfigMaps, // this can happen because objects must be downloaded by namespace/name, rather than by UID - FailSyncReasonUIDMismatchFmt = "invalid ObjectReference, UID %q does not match UID of downloaded ConfigMap %q" + FailSyncReasonUIDMismatchFmt = "invalid ConfigSource.ConfigMapRef.UID: %s does not match %s.UID: %s" // FailSyncReasonDownloadFmt is used when the download fails, e.g. due to network issues - FailSyncReasonDownloadFmt = "failed to download ConfigMap with name %q from namespace %q" + FailSyncReasonDownloadFmt = "failed to download: %s" // FailSyncReasonInformer is used when the informer fails to report the Node object FailSyncReasonInformer = "failed to read Node from informer object cache" // FailSyncReasonReset is used when we can't reset the local configuration references, e.g. due to filesystem issues FailSyncReasonReset = "failed to reset to local config" // FailSyncReasonCheckpointExistenceFmt is used when we can't determine if a checkpoint already exists, e.g. due to filesystem issues - FailSyncReasonCheckpointExistenceFmt = "failed to determine whether object with UID %q was already checkpointed" + FailSyncReasonCheckpointExistenceFmt = "failed to determine whether object %s with UID %s was already checkpointed" // FailSyncReasonSaveCheckpointFmt is used when we can't save a checkpoint, e.g. due to filesystem issues - FailSyncReasonSaveCheckpointFmt = "failed to save config checkpoint for object with UID %q" + FailSyncReasonSaveCheckpointFmt = "failed to save config checkpoint for object %s with UID %s" // FailSyncReasonSetCurrentDefault is used when we can't set the current config checkpoint to the local default, e.g. due to filesystem issues FailSyncReasonSetCurrentLocal = "failed to set current config checkpoint to local config" // FailSyncReasonSetCurrentUIDFmt is used when we can't set the current config checkpoint to a checkpointed object, e.g. due to filesystem issues - FailSyncReasonSetCurrentUIDFmt = "failed to set current config checkpoint to object with UID %q" + FailSyncReasonSetCurrentUIDFmt = "failed to set current config checkpoint to object %s with UID %s" // EmptyMessage is a placeholder in the case that we accidentally set the condition's message to the empty string. // Doing so can result in a partial patch, and thus a confusing status; this makes it clear that the message was not provided. diff --git a/test/e2e_node/dynamic_kubelet_config_test.go b/test/e2e_node/dynamic_kubelet_config_test.go index 2ab9f7d819..09debcee4f 100644 --- a/test/e2e_node/dynamic_kubelet_config_test.go +++ b/test/e2e_node/dynamic_kubelet_config_test.go @@ -86,7 +86,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Namespace: originalConfigMap.Namespace, Name: originalConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionTrue, - Message: fmt.Sprintf(status.CurRemoteMessageFmt, originalConfigMap.UID), + Message: fmt.Sprintf(status.CurRemoteMessageFmt, configMapAPIPath(originalConfigMap)), Reason: status.CurRemoteOkayReason}, expectConfig: originalKC, }, false) @@ -162,7 +162,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Name: correctConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionFalse, Message: "", - Reason: fmt.Sprintf(status.FailSyncReasonFmt, fmt.Sprintf(status.FailSyncReasonUIDMismatchFmt, "foo", correctConfigMap.UID))}, + Reason: fmt.Sprintf(status.FailSyncReasonFmt, fmt.Sprintf(status.FailSyncReasonUIDMismatchFmt, "foo", configMapAPIPath(correctConfigMap), correctConfigMap.UID))}, expectConfig: nil, event: false, }, @@ -174,7 +174,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Namespace: correctConfigMap.Namespace, Name: correctConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionTrue, - Message: fmt.Sprintf(status.CurRemoteMessageFmt, correctConfigMap.UID), + Message: fmt.Sprintf(status.CurRemoteMessageFmt, configMapAPIPath(correctConfigMap)), Reason: status.CurRemoteOkayReason}, expectConfig: correctKC, event: true, @@ -188,7 +188,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Name: failParseConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionFalse, Message: status.LkgLocalMessage, - Reason: fmt.Sprintf(status.CurFailParseReasonFmt, failParseConfigMap.UID)}, + Reason: fmt.Sprintf(status.CurFailParseReasonFmt, configMapAPIPath(failParseConfigMap))}, expectConfig: nil, event: true, }, @@ -201,7 +201,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Name: failValidateConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionFalse, Message: status.LkgLocalMessage, - Reason: fmt.Sprintf(status.CurFailValidateReasonFmt, failValidateConfigMap.UID)}, + Reason: fmt.Sprintf(status.CurFailValidateReasonFmt, configMapAPIPath(failValidateConfigMap))}, expectConfig: nil, event: true, }, @@ -244,7 +244,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Namespace: lkgConfigMap.Namespace, Name: lkgConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionTrue, - Message: fmt.Sprintf(status.CurRemoteMessageFmt, lkgConfigMap.UID), + Message: fmt.Sprintf(status.CurRemoteMessageFmt, configMapAPIPath(lkgConfigMap)), Reason: status.CurRemoteOkayReason}, expectConfig: lkgKC, event: true, @@ -257,8 +257,8 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Namespace: badConfigMap.Namespace, Name: badConfigMap.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionFalse, - Message: fmt.Sprintf(status.LkgRemoteMessageFmt, lkgConfigMap.UID), - Reason: fmt.Sprintf(status.CurFailParseReasonFmt, badConfigMap.UID)}, + Message: fmt.Sprintf(status.LkgRemoteMessageFmt, configMapAPIPath(lkgConfigMap)), + Reason: fmt.Sprintf(status.CurFailParseReasonFmt, configMapAPIPath(badConfigMap))}, expectConfig: lkgKC, event: true, }, @@ -294,7 +294,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Namespace: cm1.Namespace, Name: cm1.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionTrue, - Message: fmt.Sprintf(status.CurRemoteMessageFmt, cm1.UID), + Message: fmt.Sprintf(status.CurRemoteMessageFmt, configMapAPIPath(cm1)), Reason: status.CurRemoteOkayReason}, expectConfig: kc1, event: true, @@ -306,7 +306,7 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube Namespace: cm2.Namespace, Name: cm2.Name}}, expectConfigOK: &apiv1.NodeCondition{Type: apiv1.NodeConfigOK, Status: apiv1.ConditionTrue, - Message: fmt.Sprintf(status.CurRemoteMessageFmt, cm2.UID), + Message: fmt.Sprintf(status.CurRemoteMessageFmt, configMapAPIPath(cm2)), Reason: status.CurRemoteOkayReason}, expectConfig: kc2, event: true, @@ -488,3 +488,8 @@ func checkEvent(f *framework.Framework, desc string, expect *apiv1.NodeConfigSou return nil }, timeout, interval).Should(BeNil()) } + +// constructs the expected SelfLink for a config map +func configMapAPIPath(cm *apiv1.ConfigMap) string { + return fmt.Sprintf("/api/v1/namespaces/%s/configmaps/%s", cm.Namespace, cm.Name) +}