fix(expectation): more appropriate use of expectation #139

eggiter · 2021-07-07T03:29:18Z

More appropriate use of expectation:

use RaiseExpectations in the code to accumulate expectations since SetExpectations has no such kind of ability. reset expectation in the beginning of ReconcileJobs;
expected creation in more appropriate place;
add "expectDeletion";

More detailed comments and explanations are presented with codes.

google-cla · 2021-07-07T03:29:20Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

eggiter · 2021-07-07T03:33:34Z

@googlebot I signed it!

google-cla · 2021-07-07T03:33:39Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

eggiter · 2021-07-07T03:44:06Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot I fixed it.

eggiter · 2021-07-07T03:52:06Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign terrytangyuan after the PR has been reviewed.
You can assign the PR to them by writing /assign @terrytangyuan in a comment when ready.

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

Needs approval from an approver in each of these files:

OWNERS

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

/assign @terrytangyuan @gaocegege

terrytangyuan

LGTM. Others PTAL

Jeffwan · 2021-07-07T21:40:45Z

pkg/controller.v1/common/pod.go

+	// Creation is expected when there is no error returned
+	// We use `RaiseExpectations` here to accumulate expectations since `SetExpectations` has no such kind of ability
+	expectationPodsKey := expectation.GenExpectationPodsKey(jobKey, rt)
+	jc.Expectations.RaiseExpectations(expectationPodsKey, 1, 0)


Seems arguments to RaiseExpectation are all 1, 0 or 0, 1. Why not to use ExpectCreations directly?

Same question here.

ExpectCreations(key, 1) calls SetExpectations underneath, it will set "expectedCreation" to 1 instead of adding them(expectedCreation += 1). We are here to expect multiple creations, am i right?
As i mentioned in first comment(No. 1).

Gotcha. SGTM.

expectationPodsKey represents a group of pods. It does expect multiple creations.

Jeffwan · 2021-07-07T21:42:37Z

pkg/controller.v1/common/service.go

 	}

-	// Convert ReplicaType to lower string.
-	expectationServicesKey := expectation.GenExpectationServicesKey(jobKey, rtype)


Seems the change is to move code snippet to below? Is there a difference? line 311-313

If we expectCreation here, the informer will never observe this creation since error is returned(such as GetPortsFromJob) before service is created.
As i mentioned in first comment(No. 2).

I see, it does have some places return err before creating the services. Nice catch

Github collapse some code snippets which make the review harder. :D

Jeffwan · 2021-07-07T21:44:50Z

@eggiter Thanks for the contribution. I am curious if you meet some problems in current expectation implementation? We can better help review the changes with more context

eggiter · 2021-07-08T03:00:53Z

@eggiter Thanks for the contribution. I am curious if you meet some problems in current expectation implementation? We can better help review the changes with more context

Detailed explanations are replied.
The problems i encountered are, current implementation of "expectation" is reconciling or is waiting expectations are satisfied when there is no need to. reconciling when it should wait creation or deletion is observed, waiting when there is an error encountered and no object is created.

gaocegege

LGTM.

It's clean. Thanks for the PR.

Jeffwan · 2021-07-08T04:38:47Z

pkg/controller.v1/common/service.go

 	}

-	// Convert ReplicaType to lower string.
-	expectationServicesKey := expectation.GenExpectationServicesKey(jobKey, rtype)


I see, it does have some places return err before creating the services. Nice catch

Jeffwan · 2021-07-08T04:39:38Z

pkg/controller.v1/common/service.go

 	}

-	// Convert ReplicaType to lower string.
-	expectationServicesKey := expectation.GenExpectationServicesKey(jobKey, rtype)


Github collapse some code snippets which make the review harder. :D

Jeffwan · 2021-07-08T04:42:23Z

pkg/controller.v1/common/pod.go

+	// Creation is expected when there is no error returned
+	// We use `RaiseExpectations` here to accumulate expectations since `SetExpectations` has no such kind of ability
+	expectationPodsKey := expectation.GenExpectationPodsKey(jobKey, rt)
+	jc.Expectations.RaiseExpectations(expectationPodsKey, 1, 0)


expectationPodsKey represents a group of pods. It does expect multiple creations.

google-oss-robot · 2021-07-08T04:46:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jeffwan

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Jeffwan]

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

Jeffwan · 2021-07-08T04:47:17Z

Seems @terrytangyuan and @gaocegege both approve the PR.

/lgtm

google-oss-robot requested review from gaocegege and terrytangyuan July 7, 2021 03:29

google-oss-robot added the size/M label Jul 7, 2021

fix(expectation): more appropriate use of expection

6a3c58c

google-oss-robot assigned terrytangyuan Jul 7, 2021

eggiter changed the title ~~fix(expectation): more appropriate use of expection~~ fix(expectation): more appropriate use of expectation Jul 7, 2021

terrytangyuan reviewed Jul 7, 2021

View reviewed changes

Jeffwan reviewed Jul 7, 2021

View reviewed changes

gaocegege reviewed Jul 8, 2021

View reviewed changes

Jeffwan approved these changes Jul 8, 2021

View reviewed changes

google-oss-robot added the approved label Jul 8, 2021

google-oss-robot assigned Jeffwan Jul 8, 2021

google-oss-robot added the lgtm label Jul 8, 2021

google-oss-robot merged commit 9ffa565 into kubeflow:master Jul 8, 2021

eggiter deleted the fix-expectation branch July 8, 2021 06:22

gaocegege mentioned this pull request Jul 26, 2021

[bug] The pod wil be recreated until the expectation expires kubeflow/trainer#1306

Closed

MEllis-github mentioned this pull request Aug 22, 2023

Enhancement: handle infinite-requeuing of jobs with pod-stuck-terminating project-codeflare/multi-cluster-app-dispatcher#599

Open

fix(expectation): more appropriate use of expectation #139

fix(expectation): more appropriate use of expectation #139

Uh oh!

Conversation

eggiter commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-cla bot commented Jul 7, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

eggiter commented Jul 7, 2021

Uh oh!

google-cla bot commented Jul 7, 2021

Uh oh!

eggiter commented Jul 7, 2021

Uh oh!

eggiter commented Jul 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

terrytangyuan 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!

eggiter Jul 8, 2021 • 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!

Jeffwan commented Jul 7, 2021

Uh oh!

eggiter commented Jul 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaocegege 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!

Choose a reason for hiding this comment

Uh oh!

google-oss-robot commented Jul 8, 2021

Uh oh!

Jeffwan commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eggiter commented Jul 7, 2021 •

edited

Loading

eggiter commented Jul 7, 2021 •

edited

Loading

eggiter Jul 8, 2021 •

edited

Loading

eggiter commented Jul 8, 2021 •

edited

Loading