fix: Add option to disable Kaniko cache warming #922

asifdxtreme · 2019-09-03T13:26:18Z

Fixes: #907

This option will enable user to enable or disable the cache for building the images.

Release Note

Add the `kaniko-build-cache` option to disable Kaniko cache warming

asifdxtreme · 2019-09-03T13:28:10Z

/cc @jamesnetherton @WillemJiang

jamesnetherton · 2019-09-03T15:53:41Z

pkg/cmd/install.go

@@ -74,6 +74,7 @@ func newCmdInstall(rootCmdOptions *RootCmdOptions) *cobra.Command {
 	cmd.Flags().StringVar(&impl.buildStrategy, "build-strategy", "", "Set the build strategy")
 	cmd.Flags().StringVar(&impl.buildTimeout, "build-timeout", "", "Set how long the build process can last")
 	cmd.Flags().StringVar(&impl.traitProfile, "trait-profile", "", "The profile to use for traits")
+	cmd.Flags().BoolVar(&impl.cache, "cache", true, "To enable or disable the cache in building phase")


'cache' is perhaps a bit too vague as a flag name. Maybe align it with the other build related flags. E.g build-cache.

thanks for the review, I have updated it as per your suggestions

jamesnetherton · 2019-09-03T15:53:49Z

pkg/cmd/install.go

@@ -235,6 +237,12 @@ func (o *installCmdOptions) install(_ *cobra.Command, _ []string) error {
 			platform.Spec.Build.HTTPProxySecret = o.httpProxySecret
 		}

+		if o.cache {
+			platform.Spec.Build.Cache = true


Do we need the if statement? Can we not simplify to platform.Spec.Build.Cache = o.cache?

thanks for the review, I have updated it as per your suggestions, I was trying to be defensive if someone passes non boolean values.

jamesnetherton · 2019-09-05T07:37:50Z

pkg/apis/camel/v1alpha1/integrationplatform_types.go

@@ -105,6 +105,7 @@ type IntegrationPlatformBuildSpec struct {
 	PersistentVolumeClaim string                                  `json:"persistentVolumeClaim,omitempty"`
 	Maven                 MavenSpec                               `json:"maven,omitempty"`
 	HTTPProxySecret       string                                  `json:"httpProxySecret,omitempty"`
+	Cache                 bool                                    `json:"bool,omitempty"`


One thing I noticed when testing - the integrationplatform spec appears like:

spec: build: baseImage: fabric8/s2i-java:3.0-java8 bool: true

You'll need to tweak the field name to cache.

Thanks for the review, I have updated the code now

asifdxtreme · 2019-09-05T11:03:47Z

Can someone re-trigger this build, seems like build failed because of network failure

jamesnetherton · 2019-09-05T12:17:51Z

I think there are issues with git.apache.org so we may need to wait till they are fixed before the build is successful again.

We could maybe just merge this. I ran e2e tests locally + verified the change with minikube and it seemed ok.

asifdxtreme · 2019-09-05T12:19:29Z

I think there are issues with git.apache.org so we may need to wait till they are fixed before the build is successful again.

We could maybe just merge this. I ran e2e tests locally + verified the change with minikube and it seemed ok.

Thanks a ton @jamesnetherton and @oscerd for reviewing and verifying the changes

astefanutti · 2019-09-06T08:15:48Z

The build issue should be fixed with #926.

I would suggest the option and field are renamed more specifically to something like kanikoBuildCache.

WillemJiang · 2019-09-07T09:08:43Z

The build issue should be fixed with #926.

I would suggest the option and field are renamed more specifically to something like kanikoBuildCache.

I agree, it makes the option more easy to understand.
@asifdxtreme Could you update the patch for it?

asifdxtreme · 2019-09-11T06:33:37Z

The build issue should be fixed with #926.

I would suggest the option and field are renamed more specifically to something like kanikoBuildCache.

Thanks @astefanutti for the suggestion, I have updated the config name to kaniko-build-cache .

astefanutti · 2019-09-11T06:37:40Z

pkg/apis/camel/v1alpha1/integrationplatform_types.go

@@ -105,6 +105,7 @@ type IntegrationPlatformBuildSpec struct {
 	PersistentVolumeClaim string                                  `json:"persistentVolumeClaim,omitempty"`
 	Maven                 MavenSpec                               `json:"maven,omitempty"`
 	HTTPProxySecret       string                                  `json:"httpProxySecret,omitempty"`
+	Cache                 bool                                    `json:"cache,omitempty"`


Could you please name the API field more specifically as well.

Thanks @astefanutti for the suggestion, I have updated it now

astefanutti · 2019-09-11T06:53:43Z

pkg/controller/integrationplatform/initialize.go

@@ -137,7 +137,7 @@ func (action *initializeAction) Handle(ctx context.Context, platform *v1alpha1.I
 		}

 		// Check if the operator is running in the same namespace before starting the cache warmer
-		if platform.Namespace == platformutil.GetOperatorNamespace() {
+		if platform.Namespace == platformutil.GetOperatorNamespace() && platform.Spec.Build.KanikoBuildCache {


My understanding is that it defaults to false when the platform is not created with the kamel CLI, e.g. with Operator Hub, while it defaults to true otherwise.

The KanikoBuildCache could be a *bool, whose value is defaulted to true during the platform initialisation phase if the pointer is nil.

That makes sense, I didn't considered this scenario, I will update my code accordingly.

@astefanutti based on your input I thought to reverse the way we are dealing with this new variable, so now the kamel cli tool will take the value of skip-kaniko-build-cache whose value will be by default false. Now in the intialize we skip the cache warmer only if this value has been explicitly update as true (if platform.Namespace == platformutil.GetOperatorNamespace() && !platform.Spec.Build.SkipKanikoBuildCache {). Hope this will cover most of the scenarios of installation.

Some may find negative boolean variable a bad practice. I'm not sure Cobra handle boolean pointer so that it is possible to detect the option is not set and deal with a positive default. If it turns out it's possible, I think it'd be a better alternative.

@astefanutti I tried some options with Cobra but could'nt find a way to differentiate whether the value was not set or set to default values.

Sorry for the delay, I'm looking at it right now and I'll get back to you on this today.

It seems possible to check whether an option has been explicitly provided with flag.Flag.Changed, e.g.:

func (o *installCmdOptions) install(cmd *cobra.Command, _ []string) error { flag := cmd.Flags().Lookup("kaniko-cache") if flag.Changed { // ...

Ideally, we would need spf13/pflag#214, e.g. for knative/client#346, but that's another issue.

Thanks @astefanutti , I will look into this

Hi @astefanutti Thanks again for the suggestion and help to fix this issue, as per your review suggestions I have updated the code.

@asifdxtreme thanks for your patience. That solves the CLI part. Then when the platform is created without the CLI, the cache needs to be enabled by default. That means turning the KanikoBuildCache field into a *bool and defaulting to true when its value is nil.

astefanutti

Thanks a lot for your contribution and patience!

asifdxtreme · 2019-09-30T09:32:49Z

Thanks a lot @astefanutti for helping to find the right solution for this PR and constantly reviewing and guiding me to get this PR merged.

oscerd approved these changes Sep 3, 2019

View reviewed changes

asifdxtreme force-pushed the cache branch from 1a424ba to 9d4cc09 Compare September 3, 2019 14:05

jamesnetherton requested changes Sep 3, 2019

View reviewed changes

asifdxtreme force-pushed the cache branch from 9d4cc09 to 9324abf Compare September 4, 2019 16:44

jamesnetherton reviewed Sep 5, 2019

View reviewed changes

jamesnetherton approved these changes Sep 5, 2019

View reviewed changes

asifdxtreme force-pushed the cache branch from 17a9e4b to 13382eb Compare September 11, 2019 06:25

astefanutti reviewed Sep 11, 2019

View reviewed changes

asifdxtreme force-pushed the cache branch 2 times, most recently from 8b4a291 to 0a1f50f Compare September 11, 2019 11:05

asifdxtreme requested a review from astefanutti September 19, 2019 12:50

fix: Add option to disable Kaniko cache warming

dd43064

asifdxtreme force-pushed the cache branch from 0a1f50f to dd43064 Compare September 25, 2019 12:43

Make KanikoBuildCache *Bool

3a46366

asifdxtreme force-pushed the cache branch from 467fcb1 to 3a46366 Compare September 30, 2019 08:52

astefanutti approved these changes Sep 30, 2019

View reviewed changes

astefanutti merged commit 0ae900e into apache:master Sep 30, 2019

astefanutti mentioned this pull request Oct 1, 2019

fix: Kaniko cache default spec is not updated when calling the status endpoint #975

Merged

nicolaferraro mentioned this pull request Oct 7, 2019

Release Camel K 1.0.0-M2 #944

Closed

astefanutti added this to the 1.0.0-M2 milestone Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add option to disable Kaniko cache warming #922

fix: Add option to disable Kaniko cache warming #922

asifdxtreme commented Sep 3, 2019 •

edited by astefanutti

Loading

asifdxtreme commented Sep 3, 2019

jamesnetherton Sep 3, 2019

asifdxtreme Sep 4, 2019

jamesnetherton Sep 3, 2019

asifdxtreme Sep 4, 2019

jamesnetherton Sep 5, 2019

asifdxtreme Sep 5, 2019

asifdxtreme commented Sep 5, 2019

jamesnetherton commented Sep 5, 2019

asifdxtreme commented Sep 5, 2019

astefanutti commented Sep 6, 2019

WillemJiang commented Sep 7, 2019

asifdxtreme commented Sep 11, 2019

astefanutti Sep 11, 2019

asifdxtreme Sep 11, 2019

astefanutti Sep 11, 2019

asifdxtreme Sep 11, 2019

asifdxtreme Sep 11, 2019 •

edited

Loading

astefanutti Sep 12, 2019

asifdxtreme Sep 17, 2019

astefanutti Sep 24, 2019

astefanutti Sep 24, 2019

asifdxtreme Sep 24, 2019

asifdxtreme Sep 25, 2019

astefanutti Sep 26, 2019

astefanutti left a comment

asifdxtreme commented Sep 30, 2019

fix: Add option to disable Kaniko cache warming #922

fix: Add option to disable Kaniko cache warming #922

Conversation

asifdxtreme commented Sep 3, 2019 • edited by astefanutti Loading

asifdxtreme commented Sep 3, 2019

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

asifdxtreme commented Sep 5, 2019

jamesnetherton commented Sep 5, 2019

asifdxtreme commented Sep 5, 2019

astefanutti commented Sep 6, 2019

WillemJiang commented Sep 7, 2019

asifdxtreme commented Sep 11, 2019

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

asifdxtreme Sep 11, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefanutti left a comment

Choose a reason for hiding this comment

asifdxtreme commented Sep 30, 2019

asifdxtreme commented Sep 3, 2019 •

edited by astefanutti

Loading

asifdxtreme Sep 11, 2019 •

edited

Loading