Order ContainerStatuses the same as spec.Containers by taragu · Pull Request #7866 · knative/serving

taragu · 2020-05-06T14:41:58Z

/lint

Proposed Changes

Order ContainerStatuses so it has the same order as spec.Containers

Release Note

Order ContainerStatuses so it has the same order as spec.Containers

knative-prow-robot

@taragu: 0 warnings.

Details

In response to this:

/lint

Fixes #7658

Proposed Changes

Order ContainerStatuses so it has the same order as spec.Containers

Release Note
Order ContainerStatuses so it has the same order as spec.Containers

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

taragu · 2020-05-06T14:42:53Z

/cc @dprotaso

pkg/reconciler/revision/revision.go

julz · 2020-05-06T18:06:07Z

pkg/reconciler/revision/revision.go

-	for _, container := range rev.Spec.Containers {
+	containerIndexMap := map[string]int{}
+	for i, container := range rev.Spec.Containers {
+		containerIndexMap[container.Name] = i


one possible approach: since Go allows concurrently writing to an array from multiple goroutines as long as each individual element is only accessed from one, could we directly store to rev.Status.ContainerStatus[i] = .. in this loop (obviously making sure to do i := i and pre-create the array with the right size)?

That way we can lose this map, the channel stuff and the second loop - and any errors could be returned normally via the digestGrp.Wait().

yep that as well :)

julz · 2020-05-06T18:27:55Z

pkg/reconciler/revision/revision.go

-					image:       container.Image,
-					digestError: err,
-				}
+				rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing,


think this is racey unfortunately cos we can fail in parallel, probably need to do this after the loop

That was my suggestion to @savitaashture in the other PR to just use sync.Once, though won't help with return.

cant we just do something like

err := digestGrp.Wait(); if err != nil { rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, err) } return err

?

now that we're filling everything in line, probably no reason why not.

We can't do this because the Mark function takes a container image too. We'd need a special error type to do that.

Yah, that (special error type) is what I was suggesting in #7866 (comment). Though sync.Once would work too if necc.

julz · 2020-05-06T18:42:09Z

pkg/reconciler/revision/revision.go

-					v1.RevisionContainerMissingMessage(
-						container.Image, err.Error()))
-				return err
+				failedContainerImage = container.Image


this still races I think (there's still only one failedContainerImage potentially written by multiple goroutines), but I think we could just build and return an error containing the v1.RevisionContainerMissingMessage which we can unwrap at the bottom.

taragu · 2020-05-06T19:22:14Z

/retest

julz · 2020-05-07T13:40:21Z

pkg/reconciler/revision/revision.go

-					image:       container.Image,
-					digestError: err,
-				}
+				return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, err.Error()))


Suggested change

return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, err.Error()))

return errors.New(v1.RevisionContainerMissingMessage(container.Image, err.Error()))

Order ContainerStatuses so it has the same order as spec.Containers

vagababov · 2020-05-07T16:49:11Z

pkg/reconciler/revision/revision.go

-		})
+	if err := digestGrp.Wait(); err != nil {
+		rev.Status.MarkContainerHealthyFalse(v1.ReasonContainerMissing, err.Error())
+		rev.Status.ContainerStatuses = make([]v1.ContainerStatuses, 0)


why do we need to do this?

The TestContainerErrorMsg e2e test was failing because if we don't clear this out, we will try to create image cache so the config will fail with a different message

serving/pkg/reconciler/revision/reconcile_resources.go

Line 119 in f2d5f67

for _, container := range rev.Status.ContainerStatuses {

why not nil?

you can do this differently by using local variable for the list and only assigning it if err != nil.

knative-metrics-robot · 2020-05-07T20:00:07Z

The following is the coverage report on the affected files.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/revision/revision.go	98.1%	98.0%	-0.2

julz · 2020-05-07T20:07:07Z

/lgtm

vagababov

/lgtm

dprotaso · 2020-05-07T21:14:17Z

/approve

taragu · 2020-05-08T00:46:14Z

/retest

knative-test-reporter-robot · 2020-05-08T01:58:14Z

The following jobs failed:

Test name	Triggers	Retries
pull-knative-serving-integration-tests		0/3
pull-knative-serving-upgrade-tests	pull-knative-serving-upgrade-tests	1/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/e2e.TestSubrouteLocalSTS
test/e2e.TestSubrouteLocalSTS/fqdn

markusthoemmes

Very nice refactoring! Thanks!

/lgtm
/approve

knative-prow-robot · 2020-05-08T06:08:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, markusthoemmes, taragu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/reconciler/OWNERS~~ [dprotaso,markusthoemmes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

markusthoemmes · 2020-05-08T06:44:22Z

/retest

taragu · 2020-05-08T13:13:43Z

/retest

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 6, 2020

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/API API objects and controllers labels May 6, 2020

knative-prow-robot reviewed May 6, 2020

View reviewed changes

knative-prow-robot requested review from dgerd and mattmoor May 6, 2020 14:42

knative-prow-robot requested a review from dprotaso May 6, 2020 14:42

julz reviewed May 6, 2020

View reviewed changes

pkg/reconciler/revision/revision.go Outdated Show resolved Hide resolved

vagababov reviewed May 6, 2020

View reviewed changes

pkg/reconciler/revision/revision.go Outdated Show resolved Hide resolved

vagababov reviewed May 6, 2020

View reviewed changes

pkg/reconciler/revision/revision.go Outdated Show resolved Hide resolved

pkg/reconciler/revision/revision.go Outdated Show resolved Hide resolved

pkg/reconciler/revision/revision.go Outdated Show resolved Hide resolved

julz reviewed May 6, 2020

View reviewed changes

julz reviewed May 7, 2020

View reviewed changes

Tara Gu added 9 commits May 7, 2020 10:58

Order ContainerStatuses the same as spec.Containers

ddc9de8

Order ContainerStatuses so it has the same order as spec.Containers

Use map for sorting

4aaab52

Review

070922d

combine loops

bfce7f2

Review

c2abddd

Remove failedContainerImage

7d7f331

Fix err message

79e6368

Review

df66e17

Clear container statuses if there's an error pulling digest

19ffa32

taragu force-pushed the ordered-containerstatuses branch from 1f66322 to 19ffa32 Compare May 7, 2020 16:24

vagababov reviewed May 7, 2020

View reviewed changes

Review

ac7acb0

knative-prow-robot assigned julz May 7, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2020

vagababov reviewed May 7, 2020

View reviewed changes

knative-prow-robot assigned vagababov May 7, 2020

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020

markusthoemmes approved these changes May 8, 2020

View reviewed changes

knative-prow-robot assigned markusthoemmes May 8, 2020

knative-prow-robot merged commit 72af798 into knative:master May 8, 2020

taragu mentioned this pull request May 8, 2020

Add multi container changes to deploy PodSpec #7801

Merged

savitaashture mentioned this pull request Jun 3, 2020

[WIP] PoC for multicontainer support #6739

Closed

taragu deleted the ordered-containerstatuses branch July 29, 2020 11:40

julz mentioned this pull request Apr 15, 2021

Add julz to API Approvers knative/community#573

Merged

	return fmt.Errorf(v1.RevisionContainerMissingMessage(container.Image, err.Error()))
	return errors.New(v1.RevisionContainerMissingMessage(container.Image, err.Error()))

Conversation

taragu commented May 6, 2020

Proposed Changes

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

taragu commented May 6, 2020

Uh oh!

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!

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!

markusthoemmes May 7, 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!

julz May 7, 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!

taragu commented May 6, 2020

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!

knative-metrics-robot commented May 7, 2020

Uh oh!

julz commented May 7, 2020

Uh oh!

vagababov left a comment

Choose a reason for hiding this comment

Uh oh!

dprotaso commented May 7, 2020

Uh oh!

taragu commented May 8, 2020

Uh oh!

knative-test-reporter-robot commented May 8, 2020

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented May 8, 2020

Uh oh!

markusthoemmes commented May 8, 2020

Uh oh!

taragu commented May 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

markusthoemmes May 7, 2020 •

edited

Loading

julz May 7, 2020 •

edited

Loading