Change all Start() to Start([]namespaces) to consume updated namespaces where resources get deployed. #5487

tejal29 · 2021-03-04T00:45:20Z

Change all below Objects

pod watcher
logger
container Manager

Problem:

Watchers, Loggers are not loggings or watching resources configured in the k8 manifest config namespace if it is different than --namespace skaffold command line flag.

Reason:
Skaffold is unaware of namespaces in the k8 manifests until after we deploy. During deploy, we read manifests, and collect all namespaces.
In #2640, we updated Deploy to return a list of namespaces so all watchers, deployment fetcher can query namespaces where resources get deployed and update the runner.runCtx.Namespaces. This was used in deploy.StatusCheck however not in Logger, containerManager etc.
Associated bug #2495

In this approach, we are passing the namespace after deploy.Deploy to all Start methods to watch pods, containers in deployed namespaces.

change the signature of logger.Start and container_manager.Start and use updated namespace in runner.runCtx.Namespaces.

Pros:

The signature makes it clear we need to pass namespace

type PodWatcher interface {
	Register(receiver chan<- PodEvent)
-	Start() (func(), error)
+	Start(ns []string) (func(), error)
}

Remove namespaces from constructors for all above objects.

Alternate method

capturing the deploy time to pass on to the logger.

Pros:

small change

Cons:

Developers have to remember to capture deploy time before deploy is called and start logger after.

codecov · 2021-03-04T00:54:30Z

Codecov Report

Merging #5487 (03d1f8c) into master (31cf2f1) will decrease coverage by 0.19%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master    #5487      +/-   ##
==========================================
- Coverage   71.46%   71.26%   -0.20%     
==========================================
  Files         397      400       +3     
  Lines       14577    14971     +394     
==========================================
+ Hits        10417    10669     +252     
- Misses       3389     3513     +124     
- Partials      771      789      +18

Impacted Files	Coverage Δ
...skaffold/kubernetes/debugging/container_manager.go	`50.00% <0.00%> (ø)`
...affold/kubernetes/portforward/forwarder_manager.go	`26.66% <0.00%> (ø)`
pkg/skaffold/kubernetes/watcher.go	`84.37% <ø> (-0.48%)`	⬇️
pkg/skaffold/runner/build_deploy.go	`69.23% <ø> (+0.34%)`	⬆️
pkg/skaffold/runner/debugging.go	`66.66% <0.00%> (ø)`
pkg/skaffold/runner/dev.go	`71.52% <0.00%> (ø)`
pkg/skaffold/runner/logger.go	`40.00% <0.00%> (ø)`
pkg/skaffold/kubernetes/log.go	`47.41% <50.00%> (ø)`
...g/skaffold/kubernetes/portforward/pod_forwarder.go	`82.00% <100.00%> (ø)`
...ffold/kubernetes/portforward/resource_forwarder.go	`85.71% <100.00%> (-0.29%)`	⬇️
... and 35 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 31cf2f1...03d1f8c. Read the comment docs.

PriyaModali · 2021-03-04T16:00:22Z

LGTM.

briandealwis

I prefer option 2 and enhance PodWatcher.Start(namespaces). It would mean that when namespaces change (e.g., on a redeploy), we'd just .Stop() and then .Start(newNamespaces).

pkg/skaffold/runner/build_deploy.go

another approach.

tejal29 · 2021-03-11T01:42:18Z

Testing notes

add a namespace in any sample project

--- a/examples/getting-started/k8s-pod.yaml
+++ b/examples/getting-started/k8s-pod.yaml
@@ -2,6 +2,7 @@ apiVersion: v1
 kind: Pod
 metadata:
   name: getting-started
+  namespace: test
 spec:
   containers:
   - name: getting-started
(END)

run skaffold dev

 getting-started git:(fix_log_agg) ✗ ../../out/skaffold dev 
Listing files to watch...
 - skaffold-example
Generating tags...
 - skaffold-example -> skaffold-example:v1.20.0-31-g6b6ca7806-dirty
....
Deployments stabilized in 48.643116ms
Press Ctrl+C to exit
Watching for changes...
[getting-started] Hello world!
[getting-started] Hello world!
[getting-started] Hello world!
[getting-started] Hello world!
[getting-started] Hello world!
[getting-started] Hello world!
[getting-started] Hello world!

VS On released skaffold version ,

➜  getting-started git:(fix_log_agg) ✗ skaffold dev 
Listing files to watch...
 - skaffold-example
Generating tags...
 - skaffold-example -> skaffold-example:v1.20.0-31-g6b6ca7806-dirty
Checking cache...
 - skaffold-example: Found Locally
Tags used in deployment:
 - skaffold-example -> skaffold-example:489f1f895b4d81eb23021e673a305a87a3b6ab2e24679771c6c8255774ba0e28
Starting deploy...
 - pod/getting-started created
Waiting for deployments to stabilize...
Deployments stabilized in 19.952347ms
Press Ctrl+C to exit
Watching for changes...
^CCleaning up...
 - pod "getting-started" deleted

briandealwis · 2021-03-15T17:26:59Z

pkg/skaffold/runner/build_deploy.go

 		logrus.Warnln("Error starting port forwarding:", err)
 	}

 	// Start printing the logs after deploy is finished
-	if err := logger.Start(ctx); err != nil {
+	if err := logger.Start(ctx, r.runCtx.Namespaces); err != nil {


GetNamespaces()

(we should un-export these fields… someday)

Use

tejal29 · 2021-03-16T17:19:48Z

the CI failure is due to "Docker too many request"

Create logger and container manager instances after deploy

a74fcfc

google-cla bot added the cla: yes label Mar 4, 2021

pull-request-size bot added the size/M label Mar 4, 2021

tejal29 force-pushed the fix_log_agg branch from f113b34 to a74fcfc Compare March 4, 2021 00:45

pull-request-size bot added size/S and removed size/M labels Mar 4, 2021

tejal29 changed the title ~~Add namespace to all Start() interfaces~~ Create logger and container manager interfaces after deploy.Deploy Mar 4, 2021

tejal29 marked this pull request as ready for review March 4, 2021 00:49

tejal29 requested a review from a team as a code owner March 4, 2021 00:49

PriyaModali previously approved these changes Mar 4, 2021

View reviewed changes

briandealwis reviewed Mar 8, 2021

View reviewed changes

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

tejal29 closed this Mar 9, 2021

tejal29 mentioned this pull request Mar 9, 2021

Skipping deploy due to sync error: copying files: didn't sync any files #4246

Closed

tejal29 reopened this Mar 10, 2021

pull-request-size bot added size/L and removed size/S labels Mar 10, 2021

Use Start([]string) to all interfaces which need deployed namespaces

6b6ca78

tejal29 force-pushed the fix_log_agg branch from a53e690 to 6b6ca78 Compare March 10, 2021 23:09

tejal29 changed the title ~~Create logger and container manager interfaces after deploy.Deploy~~ Change all Start() to Start([]namespaces) to consume updated namespaces where resources get deployed. Mar 10, 2021

tejal29 added this to the v1.21.0 milestone Mar 12, 2021

briandealwis approved these changes Mar 15, 2021

View reviewed changes

tejal29 added 2 commits March 16, 2021 09:41

use GetNamespaces()

af9ad0d

Use

Update build_deploy.go

03d1f8c

tejal29 merged commit 116201a into GoogleContainerTools:master Mar 16, 2021

tejal29 mentioned this pull request Mar 31, 2021

Ensure file sync works in multi-namespace environments #5464

Closed

tejal29 deleted the fix_log_agg branch July 21, 2021 17:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change all Start() to Start([]namespaces) to consume updated namespaces where resources get deployed. #5487

Change all Start() to Start([]namespaces) to consume updated namespaces where resources get deployed. #5487

tejal29 commented Mar 4, 2021 •

edited

Loading

codecov bot commented Mar 4, 2021 •

edited

Loading

PriyaModali commented Mar 4, 2021

briandealwis left a comment

tejal29 commented Mar 11, 2021

briandealwis Mar 15, 2021

tejal29 commented Mar 16, 2021

Change all Start() to Start([]namespaces) to consume updated namespaces where resources get deployed. #5487

Change all Start() to Start([]namespaces) to consume updated namespaces where resources get deployed. #5487

Conversation

tejal29 commented Mar 4, 2021 • edited Loading

Alternate method

codecov bot commented Mar 4, 2021 • edited Loading

Codecov Report

PriyaModali commented Mar 4, 2021

briandealwis left a comment

Choose a reason for hiding this comment

tejal29 commented Mar 11, 2021

briandealwis Mar 15, 2021

Choose a reason for hiding this comment

tejal29 commented Mar 16, 2021

tejal29 commented Mar 4, 2021 •

edited

Loading

codecov bot commented Mar 4, 2021 •

edited

Loading