-
Notifications
You must be signed in to change notification settings - Fork 476
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
Optimize logging in reloader #19
Changes from 2 commits
af7e722
8c89f1a
0aa2878
d41a38e
c26ed7e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,6 @@ package handler | |
|
||
import ( | ||
"github.com/sirupsen/logrus" | ||
"k8s.io/api/core/v1" | ||
) | ||
|
||
// ResourceCreatedHandler contains new objects | ||
|
@@ -13,17 +12,7 @@ type ResourceCreatedHandler struct { | |
// Handle processes the newly created resource | ||
func (r ResourceCreatedHandler) Handle() error { | ||
if r.Resource == nil { | ||
logrus.Errorf("Error in Handler") | ||
} else { | ||
logrus.Infof("Detected changes in object %s", r.Resource) | ||
// process resource based on its type | ||
if _, ok := r.Resource.(*v1.ConfigMap); ok { | ||
logrus.Infof("A 'configmap' has been 'Added' but no implementation found to take action") | ||
} else if _, ok := r.Resource.(*v1.Secret); ok { | ||
logrus.Infof("A 'secret' has been 'Added' but no implementation found to take action") | ||
} else { | ||
logrus.Warnf("Invalid resource: Resource should be 'Secret' or 'Configmap' but found, %v", r.Resource) | ||
} | ||
logrus.Errorf("Create Handler received nil object") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is create handler @faizanahmad055 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated it to Resource creation handler received nil resource . Resource creation handler can handle the creation of new resource (i.e. Secret Or Configmap). |
||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,9 +23,8 @@ type ResourceUpdatedHandler struct { | |
// Handle processes the updated resource | ||
func (r ResourceUpdatedHandler) Handle() error { | ||
if r.Resource == nil || r.OldResource == nil { | ||
logrus.Errorf("Error in Handler") | ||
logrus.Errorf("Update Handler received nil object") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will suggest small "u" for Update |
||
} else { | ||
logrus.Infof("Detected changes in object %s", r.Resource) | ||
// process resource based on its type | ||
rollingUpgrade(r, callbacks.RollingUpgradeFuncs{ | ||
ItemsFunc: callbacks.GetDeploymentItems, | ||
|
@@ -60,23 +59,19 @@ func rollingUpgrade(r ResourceUpdatedHandler, upgradeFuncs callbacks.RollingUpgr | |
if config.SHAValue != oldSHAData { | ||
err = PerformRollingUpgrade(client, config, envVarPostfix, upgradeFuncs) | ||
if err != nil { | ||
logrus.Fatalf("Rolling upgrade failed with error = %v", err) | ||
logrus.Errorf("Rolling upgrade for %s failed with error = %v", config.ResourceName, err) | ||
} | ||
} else { | ||
logrus.Infof("Rolling upgrade will not happend because no actual change in data has been detected") | ||
} | ||
} | ||
|
||
func getConfig(r ResourceUpdatedHandler) (util.Config, string, string) { | ||
var oldSHAData, envVarPostfix string | ||
var config util.Config | ||
if _, ok := r.Resource.(*v1.ConfigMap); ok { | ||
logrus.Infof("Performing 'Updated' action for resource of type 'configmap'") | ||
oldSHAData = getSHAfromConfigmap(r.OldResource.(*v1.ConfigMap).Data) | ||
config = getConfigmapConfig(r) | ||
envVarPostfix = constants.ConfigmapEnvVarPostfix | ||
} else if _, ok := r.Resource.(*v1.Secret); ok { | ||
logrus.Infof("Performing 'Updated' action for resource of type 'secret'") | ||
oldSHAData = getSHAfromSecret(r.OldResource.(*v1.Secret).Data) | ||
config = getSecretConfig(r) | ||
envVarPostfix = constants.SecretEnvVarPostfix | ||
|
@@ -112,6 +107,8 @@ func PerformRollingUpgrade(client kubernetes.Interface, config util.Config, enva | |
var err error | ||
for _, i := range items { | ||
containers := upgradeFuncs.ContainersFunc(i) | ||
resourceName := util.ToObjectMeta(i).Name | ||
logrus.Infof("Changes Detected in %s of type '%s' in namespace: %s", config.ResourceName, envarPostfix, config.Namespace) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use small "d" for detected |
||
// find correct annotation and update the resource | ||
annotationValue := util.ToObjectMeta(i).Annotations[config.Annotation] | ||
if annotationValue != "" { | ||
|
@@ -120,13 +117,13 @@ func PerformRollingUpgrade(client kubernetes.Interface, config util.Config, enva | |
if value == config.ResourceName { | ||
updated := updateContainers(containers, value, config.SHAValue, envarPostfix) | ||
if !updated { | ||
logrus.Warnf("Rolling upgrade did not happen") | ||
logrus.Warnf("Rolling upgrade did not happen for %s of type %s in namespace: %s", resourceName, upgradeFuncs.ResourceType, config.Namespace) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why didn't it happen? shouldn't we add the reason to make the log more meaningful? |
||
} else { | ||
err = upgradeFuncs.UpdateFunc(client, config.Namespace, i) | ||
if err != nil { | ||
logrus.Errorf("Update %s failed %v", upgradeFuncs.ResourceType, err) | ||
logrus.Errorf("Update for %s of type %s in namespace %s failed with error %v", resourceName, upgradeFuncs.ResourceType, config.Namespace, err) | ||
} else { | ||
logrus.Infof("Updated %s of type %s", config.ResourceName, upgradeFuncs.ResourceType) | ||
logrus.Infof("Updated %s of type %s in namespace: %s ", resourceName, upgradeFuncs.ResourceType, config.Namespace) | ||
} | ||
break | ||
} | ||
|
@@ -139,8 +136,7 @@ func PerformRollingUpgrade(client kubernetes.Interface, config util.Config, enva | |
|
||
func updateContainers(containers []v1.Container, annotationValue string, shaData string, envarPostfix string) bool { | ||
updated := false | ||
envar := constants.EnvVarPrefix + util.ConvertToEnvVarName(annotationValue) + envarPostfix | ||
logrus.Infof("Generated environment variable: %s", envar) | ||
envar := constants.EnvVarPrefix + util.ConvertToEnvVarName(annotationValue)+ "_" + envarPostfix | ||
for i := range containers { | ||
envs := containers[i].Env | ||
|
||
|
@@ -155,7 +151,6 @@ func updateContainers(containers []v1.Container, annotationValue string, shaData | |
} | ||
containers[i].Env = append(containers[i].Env, e) | ||
updated = true | ||
logrus.Infof("%s environment variable does not exist, creating a new envVar", envar) | ||
} | ||
} | ||
return updated | ||
|
@@ -164,9 +159,7 @@ func updateContainers(containers []v1.Container, annotationValue string, shaData | |
func updateEnvVar(envs []v1.EnvVar, envar string, shaData string) bool { | ||
for j := range envs { | ||
if envs[j].Name == envar { | ||
logrus.Infof("%s environment variable found", envar) | ||
if envs[j].Value != shaData { | ||
logrus.Infof("Updating %s", envar) | ||
envs[j].Value = shaData | ||
return true | ||
} | ||
|
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.
why did we remove else section? why was it put in first place?
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.
@rasheedamir , Else section was put there to show a log statement that a new resource (i.e. secret or configmap) has been added. But it does not add any value to the reloader functionality because reloader should only show the logs when a resource has been updated.