Add events to indicate start and end of skaffold dev iterations by tejal29 · Pull Request #4037 · GoogleContainerTools/skaffold

tejal29 · 2020-04-28T06:36:03Z

/cc @quoctruong
Fixes: #4036
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

In this PR,
User facing changes (remove if N/A)
No user facing changes.
Tools consuming Event API will see following new events in the sequence.

tejaldesai@tejaldesai-macbookpro2 microservices (add_skaffold_end_event)curl localhost:50052/v1/events
{"result":{"timestamp":"2020-04-28T06:16:30.579021Z","event":{"metaEvent":{"entry":"Starting Skaffold: \u0026{Version:v1.8.0-67-ge5e6a38f3-dirty ConfigVersion:skaffold/v2beta3 GitVersion: GitCommit:e5e6a38f3abd6273ab14b6e7a71c41ec58b8b363 GitTreeState:dirty BuildDate:2020-04-27T23:16:05Z GoVersion:go1.14.1 Compiler:gc Platform:darwin/amd64}","metadata":{"build":{"numberOfArtifacts":2,"builders":[{"type":"DOCKER","count":2}],"type":"LOCAL"},"deploy":{"deployers":[{"type":"KUBECTL","count":1}],"cluster":"GKE"}}}}}}

{"result":{"timestamp":"2020-04-28T06:16:30.579067Z",
   "event":{"devLoopEvent":{"status":"In Progress"}},
  "entry":"Dev Iteration 0 in progress"}}
....

{"result":{"timestamp":"2020-04-28T06:16:35.192693Z",
   "event":{"devLoopEvent":{"status":"Succeeded"}},
   "entry":"Dev Iteration 0 successful"}}

{"result":{"timestamp":"2020-04-28T06:16:59.094247Z",
   "event":{"devLoopEvent":{"iteration":1, "changeType":"BUILD", "status":"In Progress"}},
   "entry":"Dev Iteration 1 in progress"}}

### COMPILE FAILURE - CAUSING A BUILD FAILURE
{"result":{"timestamp":"2020-04-28T06:17:03.748666Z",
   "event":{"buildEvent":{"artifact":"leeroy-app",
    "status":"Failed",
    "err":"unable to stream build output: The command '/bin/sh -c go build -o /app .' returned a 
         non-zero code: 2",
    "errCode":"ErrorCode_UNKNOWN"}},
   "entry":"Build failed for artifact leeroy-app"}}

{"result":{"timestamp":"2020-04-28T06:17:03.748736Z","
   event":{"devLoopEvent":{"iteration":1,"status":"Failed",
   "err":"couldn't build \"leeroy-app\": unable to stream build output: The command '/bin/sh -c go 
                build -o /app .' returned a non-zero code: 2",
   "errCode":"ErrorCode_UNKNOWN"}},
  "entry":"Dev Iteration 1 failed with error code ErrorCode_UNKNOWN"}}


{"result":{"timestamp":"2020-04-28T06:17:51.727711Z",
   "event":{"devLoopEvent":{"iteration":2 "changeType":"BUILD" "status":"In Progress"}},
    "entry":"Dev Iteration 2 in progress"}}
...
{"result":{"timestamp":"2020-04-28T06:18:12.363222Z",
   "event":{"devLoopEvent":{"status":"Succeeded"}},
    "entry":"Dev Iteration 2 successful"}}

{"result":{"timestamp":"2020-04-30T23:49:16.348194Z",
  "event":{
   "endEvent":{
      "errCode":1,"
        loops":[
          {"errCode":"SUCCESS"},   // 0 value for interation and changeType Initial Loop is ommitted in output
         {"iteration":1,  "changeType":"BUILD",  "errCode":"ErrorCode_UNKNOWN"}, 
         {"iteration":2,"changeType":"BUILD","errCode":"SUCCESS"}
        ]}},
     "entry":"Skaffold end event with 3 total dev loops"}}


curl: (18) transfer closed with outstanding read data remaining

Follow-up Work (remove if N/A)

[ ]Testing with IDES.

codecov · 2020-04-28T06:45:10Z

Codecov Report

Merging #4037 into master will increase coverage by 0.03%.
The diff coverage is 84.50%.

Impacted Files	Coverage Δ
pkg/skaffold/runner/runner.go	`0.00% <ø> (ø)`
pkg/skaffold/runner/dev.go	`65.11% <62.50%> (-0.38%)`	⬇️
pkg/skaffold/errors/errors.go	`66.66% <66.66%> (-33.34%)`	⬇️
pkg/skaffold/event/event.go	`93.69% <100.00%> (+0.56%)`	⬆️

pkg/skaffold/event/event.go

proto/skaffold.proto

briandealwis · 2020-04-28T13:51:38Z

proto/skaffold.proto

    STATUS_CHECK_NODE_NOT_READY= 406;

    // Unknown Error Codes
+    ErrorCode_UNKNOWN = 501;


How is this different from COULD_NOT_DETERMINE?

COULD_NOT_DETERMINE or 0 value is default enum. In cases where we don't send any error Code the value is 0. To differentiate between when it is empty Vs when we know its an unknown error, i added the field.

Can we not enforce that the error-code must be explicitly provided?

proto/skaffold.proto

pkg/skaffold/runner/dev.go

briandealwis · 2020-04-28T14:30:16Z

pkg/skaffold/errors/errors.go

-func ErrorCodeFromError(_ error, _ phase) proto.ErrorCode {
-	return proto.ErrorCode_COULD_NOT_DETERMINE
+func ErrorCodeFromError(_ error, _ Phase) proto.ErrorCode {
+	return proto.ErrorCode_ErrorCode_UNKNOWN


COULD_NOT_DETERMINE seems more apt here?

It feels like we should have an error type that carries the ErrorCode inside, rather than returning an error-code and an error. Then we could get rid of STATUS_CHECK_NO_ERROR.

I think it would make sense to add a new proto ErrorDef with those 2 fields. However, we won't be able to change that for other events BuildEvent, DeployEvent , StatusCheckEvent etc since IDEs are already consuming it.

Re: STATUS_CHECK_NO_ERR, it was something we discussed for regarding following HTTP error codes for success. It would also be easy for us to understand metrics if we define a success code vs "0" since 0 is the default value for int type enum

Looking back through this, I'm confused by the intent of this function. It's a placeholder? Can we hook in the pattern-matching aspects of #4045 here?

Or we we could our own error that carries an ErrorCode? Like the os.Err* variables and os.IsExist().

example

type ErrorWithCode interface { error ErrorCode() ErrorCode } type errorWithCode struct { errorCode ErrorCode err error } func (e errorWithCode) ErrorCode() ErrorCode { return e.errorCode } func (e errorWithCode) Error() string { return fmt.Sprintf("%d: %s", e.errorCode, e.err.Error()) } func NewErrorCode(errorCode ErrorCode, err error) ErrorWithCode { return errorWithCode{errorCode: errorCode, err: err} }

briandealwis · 2020-04-28T14:31:10Z

pkg/skaffold/errors/errors.go

+	Deploy      = Phase("Deploy")
+	StatusCheck = Phase("StatusCheck")
+	FileSync    = Phase("FileSync")
+	Dev         = Phase("Dev")


Is Dev really a phase? (We don't actually use Dev from what I can tell?)

it is not a phase, but dev-init is a stage or step in dev loop where errors can happen.
I wanted to capture this "init" step errors.

Shd "DevInit" be better?

also, i can rename "Phase" to "Stage"

Oh! So these are phases of the dev loop? That makes sense, and could you add a comment to that effect please? (It seems mildly odd for this to be defined in skaffold/errors.) I think Dev → DevInit or just Init makes sense, and perhaps we should have a TearDown too.

(I had wondered if we need to differentiate between dev and debug, but the dev loop is the same so I think it's good.)

Change Dev -> DevInit
Add TearDown
Add comment saying these are phases in dev loop.

tejal29 · 2020-04-29T05:11:40Z

Thanks a lot for all your feedback @briandealwis . I have addressed most of the comments and refactored method signatures. Defining a new struct ActionableErr makes sense so we skaffold stop (#3614) and dev iteration end event makes sense.

I am wiring up Actionable Error messages here #4045 and will propagate suggestions in the events.
Please take another look!

briandealwis

So metaEvent is the session start event, and endEvent is the session end? Are these supposed to be sent on every command or just the dev loop?

briandealwis · 2020-05-01T03:01:32Z

cmd/skaffold/app/cmd/commands.go

+		// The actual error code will be passed in
+		// https://github.com/GoogleContainerTools/skaffold/pull/4045


This doesn't seem like the right place given that the MetaEvent is raised in runner.NewForConfig(). I believe this will cause things like fix to throw session events too. (Not to mention there's ExactArgs() too.) Can we not have it be thrown at the end of the runner?

(Or if MetaEvent and EndEvent are ok for things like build, deploy, fix, etc. then could we move the start/stop here.)

briandealwis · 2020-05-01T03:08:02Z

pkg/skaffold/errors/errors.go

+	Deploy      = Phase("Deploy")
+	StatusCheck = Phase("StatusCheck")
+	FileSync    = Phase("FileSync")
+	Dev         = Phase("Dev")


Oh! So these are phases of the dev loop? That makes sense, and could you add a comment to that effect please? (It seems mildly odd for this to be defined in skaffold/errors.) I think Dev → DevInit or just Init makes sense, and perhaps we should have a TearDown too.

(I had wondered if we need to differentiate between dev and debug, but the dev loop is the same so I think it's good.)

pkg/skaffold/event/event.go

pkg/skaffold/runner/dev.go

proto/skaffold.proto

briandealwis · 2020-05-01T15:04:15Z

pkg/skaffold/errors/errors.go

-func ErrorCodeFromError(_ error, _ phase) proto.ErrorCode {
-	return proto.ErrorCode_COULD_NOT_DETERMINE
+func ErrorCodeFromError(_ error, _ Phase) proto.ErrorCode {
+	return proto.ErrorCode_ErrorCode_UNKNOWN


Looking back through this, I'm confused by the intent of this function. It's a placeholder? Can we hook in the pattern-matching aspects of #4045 here?

Or we we could our own error that carries an ErrorCode? Like the os.Err* variables and os.IsExist().

example

type ErrorWithCode interface { error ErrorCode() ErrorCode } type errorWithCode struct { errorCode ErrorCode err error } func (e errorWithCode) ErrorCode() ErrorCode { return e.errorCode } func (e errorWithCode) Error() string { return fmt.Sprintf("%d: %s", e.errorCode, e.err.Error()) } func NewErrorCode(errorCode ErrorCode, err error) ErrorWithCode { return errorWithCode{errorCode: errorCode, err: err} }

integration/rpc_test.go

cmd/skaffold/app/cmd/cmd.go

briandealwis · 2020-05-01T16:02:03Z

proto/skaffold.proto

+message ActionableErr {
+    ErrorCode errCode = 1; // error code representing the error
+    string message = 2; // message describing the error.
+    repeated string suggestions = 3; // a list of suggestions for the error.
+


I don't think reporting messages here is the right thing to do. I'm wondering if it would be better to have a secondary set of possible action codes that could be returned instead. That would also allow i18n or more.

(I think it's also an opportunity to somehow segregate PII fields, like artifact names or image tags, so that we might be able to report the actual errors seen?)

Thanks. I should have made this clear. The suggestion text is just for IDEs to show to User.
Having suggestions plumbed through skaffold will reduce the duplication of efforts in both IDEs
I will add that as a comment.
For metrics, we should probably report codes like you mentioned.

proto/skaffold.proto

tejal29 · 2020-05-04T21:27:21Z

pkg/skaffold/event/event.go

+			logEntry.Entry = fmt.Sprintf("Dev Iteration %d in progress", de.Iteration)
+		case Succeeded:
+			logEntry.Entry = fmt.Sprintf("Dev Iteration %d successful", de.Iteration)
+		default:


change this case Failed:

This reverts commit ca80757.

pkg/skaffold/event/event.go

pkg/skaffold/runner/dev.go

proto/skaffold.proto

briandealwis · 2020-05-04T23:25:24Z

docs/content/en/api/skaffold.swagger.json

+          },
+          {
+            "name": "event.devLoopEvent.err.errCode",
+            "description": " - UNKNOWN_ERROR: Could not determine error and phase\n - STATUSCHECK_SUCCESS: Status Check Success\n - STATUSCHECK_IMAGE_PULL_ERR: Container image pull error\n - STATUSCHECK_CONTAINER_CREATING: Container creating error\n - STATUSCHECK_RUN_CONTAINER_ERR: Container run error\n - STATUSCHECK_CONTAINER_TERMINATED: Container is already terminated\n - STATUSCHECK_CONTAINER_RESTARTING: Container restarting error\n - STATUSCHECK_NODE_MEMORY_PRESSURE: Node memory pressure error\n - STATUSCHECK_NODE_DISK_PRESSURE: Node disk pressure error\n - STATUSCHECK_NODE_NETWORK_UNAVAILABLE: Node network unavailable error\n - STATUSCHECK_NODE_PID_PRESSURE: Node PID pressure error\n - STATUSCHECK_NODE_UNSCHEDULABLE: Node unschedulable error\n - STATUSCHECK_NODE_UNREACHABLE: Node unreachable error\n - STATUSCHECK_NODE_NOT_READY: Node not ready error\n - STATUSCHECK_UNKNOWN: Status Check error unknown\n - STATUSCHECK_UNKNOWN_UNSCHEDULABLE: Container is unschedulable due to unknown reasons\n - STATUSCHECK_CONTAINER_WAITING_UNKNOWN: Container is waiting due to unknown reason\n - DEPLOY_UNKNOWN: Deploy failed due to unknown reason\n - SYNC_UNKNOWN: SYNC failed due to known reason\n - BUILD_UNKNOWN: Build failed due to unknown reason\n - DEVINIT_UNKNOWN: Dev Init failed due to unknown reason\n - CLEANUP_UNKNOWN: Cleanup failed due to unknown reason\n - SYNC_INIT_ERROR: File Sync Initialize failure\n - DEVINIT_REGISTER_BUILD_DEPS: Failed to configure watcher for build dependencies in dev loop\n - DEVINIT_REGISTER_TEST_DEPS: Failed to configure watcher for test dependencies in dev loop\n - DEVINIT_REGISTER_DEPLOY_DEPS: Failed to configure watcher for deploy dependencies in dev loop\n - DEVINIT_REGISTER_CONFIG_DEP: Failed to configure watcher for Skaffold configuration file.",


Can we do something to make this a more manageable string?

this was recently changed in #4083 . I will follow up later.

proto/skaffold.proto

tejal29 requested review from balopat, briandealwis, dgageot and nkubala as code owners April 28, 2020 06:36

googlebot added the cla: yes label Apr 28, 2020

pull-request-size bot added the size/XL label Apr 28, 2020

briandealwis reviewed Apr 28, 2020

View reviewed changes

tejal29 force-pushed the add_skaffold_dev_loop_events branch from 581e583 to 6e99b4c Compare April 29, 2020 05:08

tejal29 force-pushed the add_skaffold_dev_loop_events branch 3 times, most recently from 5dff70e to 6889a05 Compare April 30, 2020 23:47

pull-request-size bot added size/XXL and removed size/XL labels Apr 30, 2020

briandealwis reviewed May 1, 2020

View reviewed changes

proto/skaffold.proto Show resolved Hide resolved

pull-request-size bot added size/XL and removed size/XXL labels May 1, 2020

tejal29 commented May 4, 2020

View reviewed changes

tejal29 added 6 commits May 4, 2020 15:47

Add to indicate start and end of skaffold dev iterations

5ec9838

Add ActionableErr proto to DevLoop Event

ffb0ef8

Add test for dev loop entries

56ebbaa

add more information for proto marshall error

75fe5a9

add skaffold end session event

9cb82be

Revert "add skaffold end session event"

735e963

This reverts commit ca80757.

tejal29 force-pushed the add_skaffold_dev_loop_events branch from 5a540c2 to b467110 Compare May 4, 2020 22:54

pull-request-size bot added size/XXL and removed size/XL labels May 4, 2020

tejal29 force-pushed the add_skaffold_dev_loop_events branch from b467110 to 87a56c4 Compare May 4, 2020 22:57

brians's comments

9a2f0ff

tejal29 force-pushed the add_skaffold_dev_loop_events branch from 87a56c4 to 9a2f0ff Compare May 4, 2020 23:13

briandealwis approved these changes May 4, 2020

View reviewed changes

tejal29 added 2 commits May 4, 2020 16:50

fix diag

6411b4d

revert to master

835291c

tejal29 merged commit 53581a9 into GoogleContainerTools:master May 5, 2020

tejal29 mentioned this pull request May 5, 2020

revert back to Errcode #4112

Merged

fmotrifork mentioned this pull request May 6, 2020

skaffold 1.9.0 fishworks/fish-food#713

Merged

tejal29 mentioned this pull request May 8, 2020

[Event API] Provide skaffold dev start and end events for IDEs to consume. #4036

Closed

tejal29 deleted the add_skaffold_dev_loop_events branch April 15, 2021 07:35

		// The actual error code will be passed in
		// https://github.com/GoogleContainerTools/skaffold/pull/4045

Conversation

tejal29 commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejal29 Apr 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejal29 commented Apr 29, 2020

Uh oh!

briandealwis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tejal29 May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tejal29 commented Apr 28, 2020 •

edited

Loading

codecov bot commented Apr 28, 2020 •

edited

Loading

tejal29 Apr 28, 2020 •

edited

Loading

tejal29 May 4, 2020 •

edited

Loading