fix: prevent long startup time

[nickdapper]

Caches RunContext's kubernetes namespace so subsequent calls do not invoke kubectl.

Fixes: #8367

Description
Use my example project found here: https://github.com/nickdapper/skaffold-slow

This caches the output of the kubectl config view command which only fetches the namespace.

Please verify the location of returning the cached value is correct.

Follow-up Work (remove if N/A)
This might not be the best approach - Why is GetNamespace() called so many times?

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[ericzzzzzzz]

Hi @nickdapper , thanks for your contribution.
For caching a function call, we usually use this pattern,

skaffold/pkg/skaffold/kubectl/version.go

Lines 96 to 116 in 88e1c17

	func (c *CLI) Version(ctx context.Context) ClientVersion {
	c.versionOnce.Do(func() {
	version := Version{
	Client: ClientVersion{
	Major: unknown,
	Minor: unknown,
	},
	}
	buf, err := c.getVersion(ctx)
	if err != nil {
	warnings.Printf("unable to get kubectl client version: %v", err)
	} else if err := json.Unmarshal(buf, &version); err != nil {
	warnings.Printf("unable to parse client version: %v", err)
	}
	c.version = version.Client
	})
	return c.version
	}

This might not be the best approach - Why is GetNamespace() called so many times?

If you check the commit history around this part of code,

• 7453095
• 54d7e1a
• b2ba140
• ee13c6f

the initial implementation was just simply return namespace passed from command line, using this method was fine at that time, but when we migrated to v2, there was some knowledge gap, that caused GetNamespace() discrepancy in a lot of places, the code added was meant to fix stuff, agree we should revisit that. At this moment, I think we can cache the result to fix the start slow issue.

[nickdapper]

Thanks for the fast review @ericzzzzzzz ! I updated to use the same pattern. Let me know how this looks

[nickdapper]

@ericzzzzzzz I just tested this and while the command doesn't take two minutes however there is still a slight delay around 20 seconds before the first log. It's still calling kubectl config view 41 times in my setup.

[codecov]

Codecov Report

Merging #8376 (f52d937) into main (290280e) will decrease coverage by 4.57%.
The diff coverage is 54.41%.

@@            Coverage Diff             @@
##             main    #8376      +/-   ##
==========================================
- Coverage   70.48%   65.91%   -4.57%     
==========================================
  Files         515      605      +90     
  Lines       23150    29834    +6684     
==========================================
+ Hits        16317    19665    +3348     
- Misses       5776     8697    +2921     
- Partials     1057     1472     +415     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/completion.go	13.04% <0.00%> (-1.25%)	⬇️
cmd/skaffold/app/cmd/config/list.go	65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go	88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go	54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go	100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go	40.90% <0.00%> (-12.94%)	⬇️
cmd/skaffold/app/cmd/generate_pipeline.go	60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go	65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go	66.66% <ø> (ø)
... and 417 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ericzzzzzzz]

@nickdapper yeah.. there is another place calls kubectl config view, kubectl when deployer is initialized,

skaffold/pkg/skaffold/deploy/kubectl/kubectl.go

Lines 87 to 89 in bcb5945

	func NewDeployer(cfg Config, labeller *label.DefaultLabeller, d *latest.KubectlDeploy, artifacts []*latest.Artifact, configName string) (*Deployer, error) {
	defaultNamespace := ""
	b, err := (&util.Commander{}).RunCmdOut(context.Background(), exec.Command("kubectl", "config", "view", "--minify", "-o", "jsonpath='{..namespace}'"))

We can create a syncstore in this package

skaffold/pkg/skaffold/util/cmd.go

Lines 1 to 106 in 88e1c17

	/*
	Copyright 2019 The Skaffold Authors
	Licensed under the Apache License, Version 2.0 (the "License");
	you may not use this file except in compliance with the License.
	You may obtain a copy of the License at
	http://www.apache.org/licenses/LICENSE-2.0
	Unless required by applicable law or agreed to in writing, software
	distributed under the License is distributed on an "AS IS" BASIS,
	WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
	See the License for the specific language governing permissions and
	limitations under the License.
	*/
	package util
	import (
	"bytes"
	"context"
	"fmt"
	"os/exec"
	"github.com/GoogleContainerTools/skaffold/v2/pkg/skaffold/output/log"
	)
	type cmdError struct {
	args []string
	stdout []byte
	stderr []byte
	cause error
	}
	func (e *cmdError) Error() string {
	return fmt.Sprintf("running %s\n - stdout: %q\n - stderr: %q\n - cause: %s", e.args, e.stdout, e.stderr, e.cause)
	}
	func (e *cmdError) Unwrap() error {
	return e.cause
	}
	func (e *cmdError) ExitCode() int {
	if exitError, ok := e.cause.(*exec.ExitError); ok {
	return exitError.ExitCode()
	}
	return 0
	}
	// DefaultExecCommand runs commands using exec.Cmd
	var DefaultExecCommand Command = &Commander{}
	// Command is an interface used to run commands. All packages should use this
	// interface instead of calling exec.Cmd directly.
	type Command interface {
	RunCmdOut(ctx context.Context, cmd *exec.Cmd) ([]byte, error)
	RunCmd(ctx context.Context, cmd *exec.Cmd) error
	}
	func RunCmdOut(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
	return DefaultExecCommand.RunCmdOut(ctx, cmd)
	}
	func RunCmd(ctx context.Context, cmd *exec.Cmd) error {
	return DefaultExecCommand.RunCmd(ctx, cmd)
	}
	// Commander is the exec.Cmd implementation of the Command interface
	type Commander struct{}
	// RunCmdOut runs an exec.Command and returns the stdout and error.
	func (*Commander) RunCmdOut(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
	log.Entry(ctx).Debugf("Running command: %s", cmd.Args)
	stdout := bytes.Buffer{}
	cmd.Stdout = &stdout
	stderr := bytes.Buffer{}
	cmd.Stderr = &stderr
	if err := cmd.Start(); err != nil {
	return nil, fmt.Errorf("starting command %v: %w", cmd, err)
	}
	if err := cmd.Wait(); err != nil {
	return stdout.Bytes(), &cmdError{
	args: cmd.Args,
	stdout: stdout.Bytes(),
	stderr: stderr.Bytes(),
	cause: err,
	}
	}
	if stderr.Len() > 0 {
	log.Entry(ctx).Debugf("Command output: [%s], stderr: %s", stdout.String(), stderr.String())
	} else {
	log.Entry(ctx).Debugf("Command output: [%s]", stdout.String())
	}
	return stdout.Bytes(), nil
	}
	// RunCmd runs an exec.Command.
	func (*Commander) RunCmd(ctx context.Context, cmd *exec.Cmd) error {
	log.Entry(ctx).Debugf("Running command: %s", cmd.Args)
	return cmd.Run()
	}

syncstore implementation is here

skaffold/pkg/skaffold/util/store.go

Lines 1 to 121 in aa0bf34

	/*
	Copyright 2020 The Skaffold Authors
	Licensed under the Apache License, Version 2.0 (the "License");
	you may not use this file except in compliance with the License.
	You may obtain a copy of the License at
	http://www.apache.org/licenses/LICENSE-2.0
	Unless required by applicable law or agreed to in writing, software
	distributed under the License is distributed on an "AS IS" BASIS,
	WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
	See the License for the specific language governing permissions and
	limitations under the License.
	*/
	package util
	import (
	"fmt"
	"sync"
	"github.com/golang/groupcache/singleflight"
	)
	// SyncStore exports a single method `Exec` to ensure single execution of a function
	// and share the result between all callers of the function.
	type SyncStore[T any] struct {
	sf singleflight.Group
	results syncMap[T]
	}
	type syncMap[T any] struct {
	sync.Map
	}
	func (m *syncMap[T]) Load(k any) (v T, err error, ok bool) {
	val, found := m.Map.Load(k)
	if !found {
	return
	}
	ok = true
	switch t := val.(type) {
	case error:
	err = t
	case T:
	v = t
	}
	return
	}
	func (m *syncMap[T]) Store(k any, v T, err error) {
	if err != nil {
	m.Map.Store(k, err)
	} else {
	m.Map.Store(k, v)
	}
	}
	// Exec executes the function f if and only if it's being called the first time for a specific key.
	// If it's called multiple times for the same key only the first call will execute and store the result of f.
	// All other calls will be blocked until the running instance of f returns and all of them receive the same result.
	func (o *SyncStore[T]) Exec(key string, f func() (T, error)) (T, error) {
	val, err := o.sf.Do(key, func() (_ interface{}, err error) {
	// trap any runtime error due to synchronization issues.
	defer func() {
	if rErr := recover(); rErr != nil {
	err = retrieveError(key, rErr)
	}
	}()
	v, err, ok := o.results.Load(key)
	if ok {
	return v, err
	}
	v, err = f()
	o.results.Store(key, v, err)
	return v, err
	})
	var defaultT T
	if err != nil {
	return defaultT, err
	}
	switch t := val.(type) {
	case error:
	return defaultT, t
	case T:
	return t, nil
	default:
	return defaultT, err
	}
	}
	// Store will store the results for a key in a cache
	// This function is not safe to use if multiple subroutines store the
	// result for the same key.
	func (o *SyncStore[T]) Store(key string, r T, err error) {
	o.results.Store(key, r, err)
	}
	// NewSyncStore returns a new instance of `SyncStore`
	func NewSyncStore[T any]() *SyncStore[T] {
	return &SyncStore[T]{
	sf: singleflight.Group{},
	results: syncMap[T]{Map: sync.Map{}},
	}
	}
	// StoreError represent any error that when retrieving errors from the store.
	type StoreError struct {
	message string
	}
	func (e StoreError) Error() string {
	return e.message
	}
	func retrieveError(key string, i interface{}) StoreError {
	return StoreError{
	message: fmt.Sprintf("internal error retrieving cached results for key %s: %v", key, i),
	}
	}

And create a method like runOutOnce with wraps the runout command and use command args as key for caching purpose, something like

func RunCmdOutOnce(ctx context.Context, cmd *exec.Cmd) ([]byte, error) {
	return store.Exec(cmd.String(), func() ([]byte, error) {
		return RunCmdOut(ctx, cmd)
	})
}

and call this method when creating kubectl deployer

[nickdapper]

@ericzzzzzzz Excellent suggestion! This results in just a single call now. 🚀

Let me know if there are other changes needed.

[nickdapper]

Will commits be squashed on merge? Not seeing that.

[ericzzzzzzz]

Will commits be squashed on merge? Not seeing that.

It will

[ericzzzzzzz]

Hi @nickdapper , we need to fix the test to make the ci build succeed. :)

[nickdapper]

Hi @nickdapper , we need to fix the test to make the ci build succeed. :)

Going to work on this today. Thanks!

[nickdapper]

@ericzzzzzzz Is there a way to force a re-run of the checks?

[aaron-prindle]

@nickdapper Maintainers can re-run specific parts of the Github Actions but I don't think the PR contributor necessarily can. I think the easiest way to re-run the checks would be to re-push your commit and that will kick the CI jobs off. Also you can ping the thread here and we can kick things off again as needed (specific Github Actions, etc.)

The unit tests that are currently failing can be run locally via make tests in the skaffold/ root directory. That might be an easier way to iterate here (assuming you have access to the platform the test is failing on).

[nickdapper]

Ok all tests are passing now locally

[nickdapper]

@ericzzzzzzz The previous commit re-ran checks but not the current one. Are you able to run them again please? Should be passing now

it's running now.