feat: make jsonpath wait logics consistent with condition #109525

jonyhy96 · 2022-04-18T07:11:43Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Keep the behavior of jsonpath and condition consistent.

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1236

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubectl wait command with jsonpath flag will wait for target path appear until timeout.

k8s-triage-robot · 2022-04-18T08:06:43Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

pierDipi

Thanks for the patch!

pierDipi · 2022-04-19T07:50:20Z

staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go

+		if strings.Contains(err.Error(), "is not found") { // TODO(jonyhy96) change this to error assertion when client-go support.
+			return false, nil
+		}


Rather than parsing the error string, did you consider using AllowMissingKeys and then when len(parseResults) == 0 we can return false, nil instead of an error?

Looks great! Thanks for advice, then we can remove that todo.

jonyhy96 · 2022-04-19T10:04:39Z

/retest

pierDipi · 2022-04-19T10:11:30Z

staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go

@@ -585,10 +585,14 @@ func (j JSONPathWait) isJSONPathConditionMet(event watch.Event) (bool, error) {
 // and check if it matches the desired condition
 func (j JSONPathWait) checkCondition(obj *unstructured.Unstructured) (bool, error) {
 	queryObj := obj.UnstructuredContent()
+	j.jsonPathParser.AllowMissingKeys(true)


I would call AllowMissingKeys(true) when we create the parser [1] [2] since it changes the internal state of the parser and might produce unexpected side effects.

I wouldn't expect that x and y might be different in this case:

x, _ := j.jsonPathParser.FindResults(obj.UnstructuredContent()) j.checkCondition(obj) y, _ := j.jsonPathParser.FindResults(obj.UnstructuredContent())

[1]

func newJSONPathParser(jsonPathExpression string) (*jsonpath.JSONPath, error) { j := jsonpath.New("wait").AllowMissingKeys(true) if jsonPathExpression == "" { return nil, errors.New("jsonpath expression cannot be empty") } if err := j.Parse(jsonPathExpression); err != nil { return nil, err } return j, nil }

[2] https://github.com/kubernetes/kubernetes/blob/9c0eaddf121d91863b70b03cb408a7249b882e5e/staging/src/k8s.io/kubectl/pkg/cmd/wait/wait.go#L229-L238

Great point!

jonyhy96 · 2022-04-19T15:10:16Z

/retest

pierDipi

Thanks for the patch, LGTM.

(note: I'm not a reviewer or approver, so we need someone from the SIG for a review)

pierDipi · 2022-04-26T10:40:50Z

Since you reviewed the original PR.

/cc seans3 dims

jonyhy96 · 2022-05-16T10:28:18Z

PTAL @seans3 @dims

pierDipi · 2022-05-26T09:14:13Z

/cc @mpuckett159 @soltysh

k8s-ci-robot · 2022-05-26T09:14:14Z

@pierDipi: GitHub didn't allow me to request PR reviews from the following users: mpuckett159.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @mpuckett159 @soltysh

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.

pierDipi · 2022-06-01T12:44:33Z

test/cmd/wait.sh

+    output_message=$(kubectl wait --for=jsonpath=.status.readyReplicas=1 deploy/test-1)
+
+    # Post-Condition: Wait failed
+    kube::test::if_has_string "${output_message}" 'timed out'


shouldn't we expect condition met?

Busybox deploy will never be ready.
Change image to nginx will reach condition met, already test locally but not work as expect when running on k8s ci.

Hmmm what if the test above failed?

kube::test::get_object_assert "deployments" "{{range .items}}{{.metadata.name}},{{end}}" 'test-1,test-2,'

Hmmm what if the test above failed?

kube::test::get_object_assert "deployments" "{{range .items}}{{.metadata.name}},{{end}}" 'test-1,test-2,'

updated

dims · 2022-06-01T19:35:42Z

/assign @eddiezane

lauchokyip · 2022-06-28T23:37:36Z

/assign

lauchokyip · 2022-06-29T03:42:09Z

Yea, good call. For people who use workaround like 'until kubectl wait' would definitely see unexpected changes - longer wait time. I have informed Eddie so he can voice his opinions on this. Thanks!

…

On Jun 28, 2022, 23:38 -0400, Marcus Puckett ***@***.***>, wrote: @mpuckett159 commented on this pull request. In staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go: > @@ -1173,7 +1173,7 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) { jsonPathExp: "{.foo.bar}", jsonPathCond: "baz", - expectedErr: "foo is not found", + expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t", Sorry by middle ground I just meant keep the default for the command the way it is so it doesn't potentially break anything for downstream users, and add a flag that users can use to ignore missing keys, so that we basically enable both options. It would serve to both ensure no unexpected changes for downstream users and act as a kind of guard against users potentially adding a path to a key that doesn't exist unless they specifically are ok with it being missing. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were assigned.Message ID: ***@***.***>

Signed-off-by: jonyhy96 <hy352144278@gmail.com>

dims · 2022-08-23T12:50:20Z

/assign @eddiezane @soltysh
/milestone v1.26

neoaggelos · 2022-10-05T15:52:13Z

Hi @dims, 1.26 bug triage team here, just wanted to check whether this Is this still on track for the 1.26 milestone.

Thank you!

dims · 2022-10-05T19:24:18Z

i hope so @neoaggelos :)

jonyhy96 · 2022-10-08T03:15:42Z

please let me known if any updates i should made to push this forward! /cc @ardaguclu @lauchokyip @pierDipi

lauchokyip · 2022-10-24T18:07:21Z

@jonyhy96 I think we would have to wait for more reviewers/approvers to get this things going

soltysh

/lgtm
/approve

soltysh · 2022-10-30T10:28:31Z

staging/src/k8s.io/kubectl/pkg/cmd/wait/wait_test.go

@@ -1173,7 +1173,7 @@ func TestWaitForDifferentJSONPathExpression(t *testing.T) {
 			jsonPathExp:  "{.foo.bar}",
 			jsonPathCond: "baz",

-			expectedErr: "foo is not found",
+			expectedErr: "timed out waiting for the condition on theresource/foo-b6699dcfb-rnv7t",


The middle ground would only apply to jsonpath, b/c as mentioned in the original issue that is possible when waiting for conditions already, so I'm considering this more like a bug, and aligning the functionality at this time.

k8s-ci-robot · 2022-10-30T10:31:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jonyhy96, pierDipi, soltysh

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [soltysh]
~~test/cmd/OWNERS~~ [soltysh]

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

soltysh · 2022-10-30T10:31:49Z

Fixes kubernetes/kubectl#1204

soltysh · 2022-10-30T10:32:12Z

/triage accepted
/priority important-longterm

soltysh · 2022-10-30T10:32:26Z

/remove-label kind/api-change

k8s-ci-robot · 2022-10-30T10:32:28Z

@soltysh: The label(s) /remove-label kind/api-change cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, official-cve-feed. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/remove-label kind/api-change

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.

pacoxu · 2022-10-30T13:33:25Z

/retest

k8s-ci-robot requested review from brianpursley and eddiezane April 18, 2022 07:11

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 18, 2022

jonyhy96 force-pushed the feat-wait branch from 4dc8b66 to 1ff570b Compare April 18, 2022 07:14

pierDipi reviewed Apr 19, 2022

View reviewed changes

jonyhy96 force-pushed the feat-wait branch from 1ff570b to 9c0eadd Compare April 19, 2022 08:32

pierDipi reviewed Apr 19, 2022

View reviewed changes

jonyhy96 force-pushed the feat-wait branch from 9c0eadd to a9fbc9f Compare April 19, 2022 11:19

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 19, 2022

pierDipi approved these changes Apr 26, 2022

View reviewed changes

k8s-ci-robot requested review from dims and seans3 April 26, 2022 10:40

pierDipi reviewed Jun 1, 2022

View reviewed changes

k8s-ci-robot assigned eddiezane Jun 1, 2022

k8s-ci-robot assigned lauchokyip Jun 28, 2022

feat: make jsonpath wait logics consistent with condition

7a1493b

Signed-off-by: jonyhy96 <hy352144278@gmail.com>

jonyhy96 force-pushed the feat-wait branch from 88f3662 to 7a1493b Compare June 29, 2022 04:28

k8s-ci-robot assigned soltysh Aug 23, 2022

k8s-ci-robot added this to the v1.26 milestone Aug 23, 2022

soltysh approved these changes Oct 30, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2022

k8s-ci-robot merged commit 526650f into kubernetes:master Oct 30, 2022

jonyhy96 deleted the feat-wait branch November 1, 2022 05:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make jsonpath wait logics consistent with condition #109525

feat: make jsonpath wait logics consistent with condition #109525

jonyhy96 commented Apr 18, 2022 •

edited

k8s-triage-robot commented Apr 18, 2022

pierDipi left a comment

pierDipi Apr 19, 2022

jonyhy96 Apr 19, 2022

jonyhy96 commented Apr 19, 2022

pierDipi Apr 19, 2022

jonyhy96 Apr 19, 2022

jonyhy96 commented Apr 19, 2022

pierDipi left a comment •

edited

pierDipi commented Apr 26, 2022

jonyhy96 commented May 16, 2022

pierDipi commented May 26, 2022

k8s-ci-robot commented May 26, 2022

pierDipi Jun 1, 2022

jonyhy96 Jun 2, 2022

lauchokyip Jun 29, 2022 •

edited

jonyhy96 Jun 29, 2022

dims commented Jun 1, 2022

lauchokyip commented Jun 28, 2022

lauchokyip commented Jun 29, 2022 via email

dims commented Aug 23, 2022

neoaggelos commented Oct 5, 2022

dims commented Oct 5, 2022

jonyhy96 commented Oct 8, 2022

lauchokyip commented Oct 24, 2022

soltysh left a comment

soltysh Oct 30, 2022

k8s-ci-robot commented Oct 30, 2022

soltysh commented Oct 30, 2022

soltysh commented Oct 30, 2022

soltysh commented Oct 30, 2022

k8s-ci-robot commented Oct 30, 2022

pacoxu commented Oct 30, 2022

feat: make jsonpath wait logics consistent with condition #109525

feat: make jsonpath wait logics consistent with condition #109525

Conversation

jonyhy96 commented Apr 18, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-triage-robot commented Apr 18, 2022

pierDipi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonyhy96 commented Apr 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonyhy96 commented Apr 19, 2022

pierDipi left a comment • edited

Choose a reason for hiding this comment

pierDipi commented Apr 26, 2022

jonyhy96 commented May 16, 2022

pierDipi commented May 26, 2022

k8s-ci-robot commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lauchokyip Jun 29, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dims commented Jun 1, 2022

lauchokyip commented Jun 28, 2022

lauchokyip commented Jun 29, 2022 via email

dims commented Aug 23, 2022

neoaggelos commented Oct 5, 2022

dims commented Oct 5, 2022

jonyhy96 commented Oct 8, 2022

lauchokyip commented Oct 24, 2022

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 30, 2022

soltysh commented Oct 30, 2022

soltysh commented Oct 30, 2022

soltysh commented Oct 30, 2022

k8s-ci-robot commented Oct 30, 2022

pacoxu commented Oct 30, 2022

jonyhy96 commented Apr 18, 2022 •

edited

pierDipi left a comment •

edited

lauchokyip Jun 29, 2022 •

edited