Add StatusCheck field to skaffold.yaml (#4904) #5706

maggieneterval · 2021-04-19T19:17:58Z

Fixes: #4904
Merge before/after: Requires rebase after #5700, #5712, #5748 are merged.

Description
Add StatusCheck field to skaffold.yaml.

User facing changes

Before:

--status-check flag	healthcheck enabled?
unset	yes
true	yes
false	no

After:

--status-check flag	StatusCheck in skaffold.yaml	healthcheck enabled?
unset	unset	yes
unset	false	no
unset	true	yes
false	unset	no
false	false	no
false	true	no
true	unset	yes
true	false	yes
true	true	yes

gsquared94 · 2021-04-19T22:15:47Z

From the linked issue:

Upon further investigation, it appears that there is no way to distinguish a true from an unset --status-check flag value (unset defaults to true).

Can we change the type of status-check from a boolean to a custom type with the three states True, False and UnSpecified. Otherwise I think that this status-check stands out as an exception to the behavior of all the other flags that override what is specified in the skaffold.yaml.

MarlonGamez · 2021-04-19T22:23:25Z

@maggieneterval @gsquared94 there's a newly introduced Nillable interface to use for types similar to what you're suggesting

skaffold/cmd/skaffold/app/cmd/flags.go

Lines 37 to 42 in bf8685a

    
           // Nillable is used to reset objects that implement pflag's `Value` and `SliceValue`. 
        
           // Some flags, like `--default-repo`, use nil to indicate that they are unset, which 
        
           // is different from the empty string. 
        
           type Nillable interface { 
        
           	SetNil() error 
        
           }

You can see an example implementation on the StringOrUndefined type in pkg/skaffold/config/types.go:

skaffold/pkg/skaffold/config/types.go

Lines 23 to 29 in bf8685a

    
           // StringOrUndefined holds the value of a flag of type `string`, 
        
           // that's by default `undefined`. 
        
           // We use this instead of just `string` to differentiate `undefined` 
        
           // and `empty string` values. 
        
           type StringOrUndefined struct { 
        
           	value *string 
        
           }

maggieneterval · 2021-04-20T18:50:42Z

Thanks for the comments, @gsquared94 and @marlon-gamez! In the spirit of small PRs, I will open a separate PR to make the status-check flag implement Nillable and then rebase this PR on that commit.

maggieneterval · 2021-04-23T20:08:22Z

Hi @marlon-gamez & @gsquared94, this should nearly be ready for review as I've rebased it off #5712. Just need to resolve an integration test error: Type mismatch in profile overlay for field 'statusCheck' with type bool; falling back to original config values that occurs when running skaffold while specifying a profile. Will continue to debug this but please let me know if I've overlooked something obvious! Thank you!

MarlonGamez · 2021-04-23T21:15:02Z

Hey @maggieneterval, I think this issue is caused by this function not having a case for handling bools.

skaffold/pkg/skaffold/schema/profiles.go

Lines 311 to 348 in f280aa8

    
           func overlayProfileField(fieldName string, config interface{}, profile interface{}) interface{} { 
        
           	v := reflect.ValueOf(profile) // the profile itself 
        
           	t := reflect.TypeOf(profile)  // the type of the profile, used for getting struct field types 
        
           	logrus.Debugf("overlaying profile on config for field %s", fieldName) 
        
           	switch v.Kind() { 
        
           	case reflect.Struct: 
        
           		// check the first field of the struct for a oneOf yamltag. 
        
           		if util.IsOneOfField(t.Field(0)) { 
        
           			return overlayOneOfField(config, profile) 
        
           		} 
        
           		return overlayStructField(config, profile) 
        
           	case reflect.Slice: 
        
           		// either return the values provided in the profile, or the original values if none were provided. 
        
           		if v.Len() == 0 { 
        
           			return config 
        
           		} 
        
           		return v.Interface() 
        
           	case reflect.Ptr: 
        
           		// either return the values provided in the profile, or the original values if none were provided. 
        
           		if v.IsNil() { 
        
           			return config 
        
           		} 
        
           		return v.Interface() 
        
           	case reflect.Int: 
        
           		if v.Interface() == reflect.Zero(v.Type()).Interface() { 
        
           			return config 
        
           		} 
        
           		return v.Interface() 
        
           	case reflect.String: 
        
           		if reflect.DeepEqual("", v.Interface()) { 
        
           			return config 
        
           		} 
        
           		return v.Interface() 
        
           	default: 
        
           		logrus.Fatalf("Type mismatch in profile overlay for field '%s' with type %s; falling back to original config values", fieldName, v.Kind()) 
        
           		return config 
        
           	} 
        
           }

I'm actually curious about how this wasn't caught before with other bool fields. I'm asking the team about why this is and I'll get back to you

MarlonGamez · 2021-04-23T21:52:05Z

It seems like we'll need to update this function to handle bool values as well. Trying locally, it seems that just adding

	case reflect.Bool:
		if v.Interface() == reflect.Zero(v.Type()).Interface() {
			return config
		}
		return v.Interface()

should be good

gsquared94 · 2021-04-23T22:08:35Z

pkg/skaffold/schema/latest/config.go

@@ -497,6 +497,9 @@ type TestCase struct {
 type DeployConfig struct {
 	DeployType `yaml:",inline"`

+	// StatusCheck *beta* enables waiting for deployments to stabilize.
+	StatusCheck bool `yaml:"statusCheck,omitempty"`


I think omitempty only works if this is *bool

also, this needs to be *bool anyways. Otherwise it'll get a value of false by default and you can't distinguish if that value was set by the user or not (same problem as with the status-check flag that you just fixed).

👍 Updated! (My initial implementation had skaffold fix automatically add a StatusCheck: true stanza to the skaffold.yaml to preserve the existing enabled-by-default behavior, so I also removed that logic.)

gsquared94 · 2021-04-23T22:20:23Z

pkg/skaffold/runner/runcontext/context.go

+	scOpts := rc.Opts.StatusCheck.Value()
+	scPipelines := rc.Pipelines.StatusCheck()
+	if scOpts == nil {
+		return scPipelines
 	}
-	return *sc
+	return *scOpts


this logic breaks current behavior?

eg: If I don't define statusCheck in my skaffold.yaml it'll be set to false. Since I don't set the flag either, it'll skip status check (line 164).

it should rather be:

scOpts := rc.Opts.StatusCheck.Value() scConfig := rc.Pipelines.StatusCheck() switch { case scOpts != nil: return *scOpts case scConfig != nil: return *scConfig default: return true }

👍 Updated! (My initial implementation had skaffold fix automatically add a StatusCheck: true stanza to the skaffold.yaml in order to not break the enabled-by-default behavior, but changing its type to *bool instead to distinguish false from unspecified is much better, thanks!)

pkg/skaffold/runner/deploy_test.go

codecov · 2021-04-26T15:31:48Z

Codecov Report

Merging #5706 (75ace97) into master (03e9a20) will decrease coverage by 0.02%.
The diff coverage is 62.50%.

@@            Coverage Diff             @@
##           master    #5706      +/-   ##
==========================================
- Coverage   70.68%   70.65%   -0.03%     
==========================================
  Files         423      428       +5     
  Lines       16168    16193      +25     
==========================================
+ Hits        11428    11441      +13     
- Misses       3900     3908       +8     
- Partials      840      844       +4

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/v1/config.go	`58.82% <ø> (ø)`
pkg/skaffold/schema/latest/v2/config.go	`0.00% <0.00%> (ø)`
pkg/skaffold/schema/latest/v2/upgrade.go	`0.00% <0.00%> (ø)`
pkg/skaffold/schema/v3alpha1/config.go	`0.00% <0.00%> (ø)`
pkg/skaffold/schema/v3alpha1/upgrade.go	`0.00% <0.00%> (ø)`
pkg/skaffold/runner/deploy.go	`53.00% <50.00%> (-0.61%)`	⬇️
pkg/skaffold/runner/runcontext/context.go	`83.82% <81.81%> (-0.93%)`	⬇️
pkg/skaffold/util/tar.go	`50.66% <0.00%> (-5.34%)`	⬇️
pkg/skaffold/kubernetes/log.go
pkg/skaffold/kubernetes/image_list.go	`100.00% <0.00%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5acad2...75ace97. Read the comment docs.

maggieneterval · 2021-04-26T15:37:37Z

pkg/skaffold/runner/runcontext/context.go

+func (ps Pipelines) StatusCheck() *bool {
+	var sc *bool
+	// set the group status check to disabled if any pipeline has StatusCheck
+	// set to false.


I was not sure what would be reasonable here; how/why would a user set only a single pipeline's StatusCheck value to false? (Not 100% clear on the relationship between a skaffold.yaml file, a profile, and a pipeline, to be honest...)

I'm not completely sure the reason a user would disable it in one, @gsquared94 might know a situation tho.

a skaffold.yaml can contain one or more skaffod configs. A config defines a pipeline so that skaffold knows how to build, test, deploy, and do statuscheck for a project. A config can also define profiles that replace parts of main config when the user activates a profile

a skaffold.yaml can contain one or more skaffod configs. A config defines a pipeline

Ah, that makes sense. Thank you for clarifying! Let me know if it would be more reasonable to (1) disable healthcheck if any pipeline has StatusCheck set to false, or (2) enable healthcheck unless all pipelines have StatusCheck set to false. Thanks again for all your help with this!

had a small discussion on this out of band, but this is probably a new scenario where we want to have values set on a per-config basis. there are surely situations where users want to enable health check for a single module, but leave it disabled for others (e.g. if one module is deploying something long running background task, health checks aren't useful), so it seems strange to disable it altogether if one particular module wants it off.

looking at how we have the code structured around the RunContext right now, this isn't a trivial change, so in interest of not completely blocking this implementation I think we can do this in a follow up PR. in the meantime, we should do two things here:

disable the health check if any pipelines explicitly disable it, since the alternative would be enabling it for long-running deployments that could cause frustrating UX

error out if users are explicitly setting status check configuration in one pipeline, and explicitly disabling it in any other. this will prevent surprising behavior, and at least give users a thread they can follow to get things to work in a less confusing way (we should probably point to docs)

@maggieneterval would you mind also creating an issue to track the follow up work here, and tie it back to this PR? something like "Support configuring status check independently between modules"

sorry I was out for a few days, but @nkubala's point above is exactly what I wanted to say.

Thank you both so much for your help with this. I opened #5746 to document this follow-up work.

MarlonGamez · 2021-04-28T22:52:38Z

hey @maggieneterval, we just did a release and we need to generate a new schema version. Once #5748 goes in you'll just need to rebase on it and I think this PR should pretty much be good to merge after that!

…et to false

maggieneterval · 2021-04-30T16:05:18Z

This should be ready for a final look. Thank you! 😃

pkg/skaffold/runner/runcontext/context.go

tejal29

Just a small nit, you can short circuit the enabled && disabled check and return early if both are set in the for loop.

maggieneterval · 2021-04-30T19:51:32Z

CI is now failing with the following error:

anyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

Is this a known issue? I can try re-triggering the CI on Monday.

tejal29 · 2021-04-30T21:12:58Z

CI is now failing with the following error:
anyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit
Is this a known issue? I can try re-triggering the CI on Monday.

I will try retrying now. But yes this has been happening a lot lately and @nkubala did some digging into it.

maggieneterval requested a review from a team as a code owner April 19, 2021 19:17

pull-request-size bot added the size/M label Apr 19, 2021

google-cla bot added the cla: yes label Apr 19, 2021

maggieneterval mentioned this pull request Apr 19, 2021

Add status-check option to the skaffold.yaml #4904

Closed

maggieneterval mentioned this pull request Apr 21, 2021

Make status-check flag nillable #5712

Merged

maggieneterval force-pushed the status-check branch from 1b98fe4 to a6414f9 Compare April 23, 2021 15:37

pull-request-size bot added size/L and removed size/M labels Apr 23, 2021

gsquared94 reviewed Apr 23, 2021

View reviewed changes

pkg/skaffold/runner/deploy_test.go Outdated Show resolved Hide resolved

maggieneterval commented Apr 26, 2021

View reviewed changes

MarlonGamez added the kokoro:run runs the kokoro jobs on a PR label Apr 26, 2021

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 26, 2021

maggieneterval mentioned this pull request Apr 28, 2021

Support configuring status check independently between modules #5746

Open

maggieneterval force-pushed the status-check branch from 63dc3ba to 7b40a1a Compare April 28, 2021 19:56

MarlonGamez mentioned this pull request Apr 28, 2021

Update hack/new_version.sh script and generate v2beta16 #5748

Merged

maggieneterval added 4 commits April 30, 2021 09:32

Add StatusCheck field to skaffold.yaml (GoogleContainerTools#4904)

d593bdf

Change StatusCheck field from bool to *bool

5231453

Fail if one pipeline has StatusCheck set to true and another has it s…

15ad574

…et to false

Move new statusCheck field from v2beta15.json to v2beta16.json

f7627f8

maggieneterval force-pushed the status-check branch from 7b40a1a to f7627f8 Compare April 30, 2021 15:32

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Apr 30, 2021

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 30, 2021

tejal29 reviewed Apr 30, 2021

View reviewed changes

pkg/skaffold/runner/runcontext/context.go Outdated Show resolved Hide resolved

tejal29 reviewed Apr 30, 2021

View reviewed changes

tejal29 approved these changes Apr 30, 2021

View reviewed changes

Short circuit the enabled && disabled StatusCheck check

75ace97

maggieneterval force-pushed the status-check branch from 257d6e8 to 75ace97 Compare April 30, 2021 18:57

tejal29 enabled auto-merge (squash) April 30, 2021 21:13

tejal29 added kokoro:force-run forces a kokoro re-run on a PR kokoro:run runs the kokoro jobs on a PR labels Apr 30, 2021

kokoro-team removed kokoro:run runs the kokoro jobs on a PR kokoro:force-run forces a kokoro re-run on a PR labels Apr 30, 2021

tejal29 merged commit ebe2f6d into GoogleContainerTools:master Apr 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add StatusCheck field to skaffold.yaml (#4904) #5706

Add StatusCheck field to skaffold.yaml (#4904) #5706

maggieneterval commented Apr 19, 2021 •

edited

Loading

gsquared94 commented Apr 19, 2021

MarlonGamez commented Apr 19, 2021

maggieneterval commented Apr 20, 2021

maggieneterval commented Apr 23, 2021

MarlonGamez commented Apr 23, 2021

MarlonGamez commented Apr 23, 2021

gsquared94 Apr 23, 2021

gsquared94 Apr 23, 2021

maggieneterval Apr 26, 2021

gsquared94 Apr 23, 2021

gsquared94 Apr 23, 2021

maggieneterval Apr 26, 2021

codecov bot commented Apr 26, 2021 •

edited

Loading

maggieneterval Apr 26, 2021

MarlonGamez Apr 26, 2021

maggieneterval Apr 27, 2021

nkubala Apr 28, 2021

gsquared94 Apr 28, 2021

maggieneterval Apr 28, 2021

MarlonGamez commented Apr 28, 2021

maggieneterval commented Apr 30, 2021

tejal29 left a comment

maggieneterval commented Apr 30, 2021

tejal29 commented Apr 30, 2021

Add StatusCheck field to skaffold.yaml (#4904) #5706

Add StatusCheck field to skaffold.yaml (#4904) #5706

Conversation

maggieneterval commented Apr 19, 2021 • edited Loading

gsquared94 commented Apr 19, 2021

MarlonGamez commented Apr 19, 2021

maggieneterval commented Apr 20, 2021

maggieneterval commented Apr 23, 2021

MarlonGamez commented Apr 23, 2021

MarlonGamez commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 26, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarlonGamez commented Apr 28, 2021

maggieneterval commented Apr 30, 2021

tejal29 left a comment

Choose a reason for hiding this comment

maggieneterval commented Apr 30, 2021

tejal29 commented Apr 30, 2021

maggieneterval commented Apr 19, 2021 •

edited

Loading

codecov bot commented Apr 26, 2021 •

edited

Loading