OCPBUGS-11652: Extend adm node-logs to new API by aravindhp · Pull Request #1403 · openshift/oc

aravindhp · 2023-04-14T23:19:32Z

Map --tail internally to tailLines
Map --unit internally to query
Use service instead of journal in the help to normalize across Linux and Windows

openshift-ci · 2023-04-14T23:19:35Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2023-04-14T23:19:37Z

@aravindhp: This pull request references Jira Issue OCPBUGS-11652, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @zhouying7780

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Add support for --query, --since-time, --until-time, --tail-lines

Map --tail to --tail-lines

Map --unit to --query

Use service instead of journal in the help to normalize across Linux and Windows

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.

aravindhp · 2023-04-14T23:20:56Z

/test unit
/test verify
/test verify-deps
/test e2e-aws-ovn

aravindhp · 2023-04-14T23:21:45Z

/cc @ardaguclu

aravindhp · 2023-04-14T23:23:08Z

/cc @soltysh

ardaguclu

@aravindhp thanks for the PR. I left a few comments.

ardaguclu · 2023-04-17T06:56:02Z

 		return fmt.Errorf("--boot accepts values [-100, 0]")
 	}
+	if len(o.Query) > 0 && (len(o.Path) > 0 || len(o.Units) > 0) {
+		return fmt.Errorf("cannot combine --query with --unit or --path")


Suggested change

return fmt.Errorf("cannot combine --query with --unit or --path")

return fmt.Errorf("--path and --unit flags are mutually exclusive with --query")

aravindhp

Thanks for the review, @ardaguclu. I have responded to your comments.

aravindhp · 2023-04-17T16:10:53Z

 		return fmt.Errorf("--boot accepts values [-100, 0]")
 	}
+	if len(o.Query) > 0 && (len(o.Path) > 0 || len(o.Units) > 0) {
+		return fmt.Errorf("cannot combine --query with --unit or --path")


ardaguclu

If the user sets query, we are setting path to empty string. But we are generating Path by using path flag in here

oc/pkg/cli/admin/node/logs.go

Line 253 in a65a29a

    
           Resource(mapping.Resource.Resource).SubResource("proxy", "logs").Suffix(o.Path).URL().Path

, will it work for the new API?

ardaguclu · 2023-04-18T05:52:52Z

+		return fmt.Errorf("cannot combine --since-time with --since or --until")
+	}
+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {
+		return fmt.Errorf("cannot combine --since-time with --since or --until")


Can we also change this in the format of https://github.com/openshift/oc/pull/1403/files#diff-fe5df7f76dde8b46f44cbcfa239451ec304c914cfc3d6a17087debb317079ee0R194

ardaguclu · 2023-04-18T05:53:25Z

+		return fmt.Errorf("--path and --unit flags are mutually exclusive with --query")
+	}
+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {
+		return fmt.Errorf("cannot combine --since-time with --since or --until")


Can we also change this in the format of https://github.com/openshift/oc/pull/1403/files#diff-fe5df7f76dde8b46f44cbcfa239451ec304c914cfc3d6a17087debb317079ee0R194?

ardaguclu · 2023-04-18T05:54:17Z

+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {
+		return fmt.Errorf("cannot combine --since-time with --since or --until")
+	}
+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {


I think this should be len(o.UntilTime)?

Sorry. Fixed.

aravindhp

If the user sets query, we are setting path to empty string. But we are generating Path by using path flag in here

oc/pkg/cli/admin/node/logs.go

Line 253 in a65a29a

Resource(mapping.Resource.Resource).SubResource("proxy", "logs").Suffix(o.Path).URL().Path

, will it work for the new API?

Yes that will still work. Setting path to journal means the API call will be proxy/logs/journal?options=. With query it will be proxy/logs/?query=... and that works.

aravindhp · 2023-04-18T19:34:49Z

+		return fmt.Errorf("--path and --unit flags are mutually exclusive with --query")
+	}
+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {
+		return fmt.Errorf("cannot combine --since-time with --since or --until")


aravindhp · 2023-04-18T19:35:00Z

+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {
+		return fmt.Errorf("cannot combine --since-time with --since or --until")
+	}
+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {


Sorry. Fixed.

aravindhp · 2023-04-18T19:35:06Z

+		return fmt.Errorf("cannot combine --since-time with --since or --until")
+	}
+	if len(o.SinceTime) > 0 && (len(o.Since) > 0 || len(o.Until) > 0) {
+		return fmt.Errorf("cannot combine --since-time with --since or --until")


ardaguclu · 2023-04-19T06:48:35Z

Sounds good to me, thanks @aravindhp

aravindhp · 2023-04-24T21:17:14Z

/test e2e-agnostic-ovn-cmd

The failure is in [sig-instrumentation] Prometheus [apigroup:image.openshift.io] when installed on the cluster shouldn't report any alerts in firing state apart from Watchdog and AlertmanagerReceiversNotConfigured and has nothing to do with the changes in this PR.

aravindhp · 2023-04-24T23:18:08Z

/retest-required

e2e-aws-ovn-serial failed in [sig-arch] events should not repeat pathologically which has nothing to do with this PR

ardaguclu · 2023-04-28T05:31:13Z

/test e2e-metal-ipi-ovn-ipv6
/test e2e-aws-ovn-serial

ardaguclu · 2023-05-09T10:49:27Z

According to the pre-merging tests, this PR is compatible with older server versions. I think, we can merge it especially considering that current oc adm node-logs is not working already.

/lgtm
/hold
feel free to cancel hold tag

soltysh

/lgtm cancel

aravindhp

Thanks for the review, @soltysh. I will address your requests shortly.

aravindhp · 2023-05-09T22:02:11Z

@soltysh I have addressed your comments. Please review.

aravindhp · 2023-05-10T01:46:15Z

/retest-required

Cluster failed to come up e2e-aws-ovn-upgrade

openshift-ci-robot · 2023-05-10T18:09:02Z

@aravindhp: This pull request references Jira Issue OCPBUGS-11652, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.14.0) matches configured target version for branch (4.14.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @zhouying7780

Details

In response to this:

Map --tail internally to tailLines

Map --unit internally to query

Use service instead of journal in the help to normalize across Linux and Windows

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.

zhouying7780 · 2023-05-17T03:13:35Z

 		oc adm node-logs --role master --path=cron
+
+		# Display kubelet log from worker nodes using the new query sub-command
+		oc adm node-logs --role worker --query=kubelet


Now , we can't support query, please remove this example.

Sorry about that. I have now removed it.

- Map --tail internally to tailLines - Map --unit internally to query - Use service instead of journal in the help to normalize across Linux and Windows

ardaguclu · 2023-05-31T05:48:45Z

@aravindhp What are we waiting for this PR?, is it requiring a new round of review?

aravindhp · 2023-05-31T15:00:15Z

@ardaguclu it is not waiting on anything other than the official QE ack.

/hold cancel
/retest-required

ardaguclu · 2023-06-01T07:08:01Z

I think, we are waiting for the last review from @soltysh;
/test e2e-metal-ipi-ovn-ipv6

ardaguclu · 2023-06-01T10:59:54Z

@aravindhp I have one last question just for clarification;
will this https://github.com/openshift/origin/blob/6aeee601dea8ecb548c251f6a457ad66f0e44e77/pkg/monitor/nodedetails/node_log.go#L37 continue working after this PR?

aravindhp · 2023-06-01T14:37:16Z

Yes the relative time options for --since and --until will continue to work. QE and I have manually have verified this.

ardaguclu · 2023-06-05T07:44:16Z

/test e2e-metal-ipi-ovn-ipv6

soltysh

/lgtm

openshift-ci · 2023-06-05T14:33:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aravindhp, ardaguclu, soltysh

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:

~~OWNERS~~ [ardaguclu,soltysh]

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

aravindhp · 2023-06-05T16:21:19Z

/retest-required

openshift-ci-robot · 2023-06-05T17:46:51Z

/retest-required

Remaining retests: 0 against base HEAD 60caade and 2 for PR HEAD a99b3f0 in total

openshift-ci · 2023-06-05T20:04:46Z

@aravindhp: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

openshift-ci-robot · 2023-06-05T20:08:48Z

@aravindhp: Jira Issue OCPBUGS-11652: Some pull requests linked via external trackers have merged:

openshift/origin#27867

The following pull requests linked via external trackers have not merged:

openshift/oc#1392 is closed

These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with /jira refresh.

Jira Issue OCPBUGS-11652 has not been moved to the MODIFIED state.

Details

In response to this:

Map --tail internally to tailLines

Map --unit internally to query

Use service instead of journal in the help to normalize across Linux and Windows

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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 14, 2023

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2023

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 14, 2023

openshift-ci Bot requested a review from zhouying7780 April 14, 2023 23:19

openshift-ci Bot requested a review from ardaguclu April 14, 2023 23:21

openshift-ci Bot requested a review from soltysh April 14, 2023 23:23

ardaguclu reviewed Apr 17, 2023

View reviewed changes

aravindhp force-pushed the OCPBUGS-11652 branch from 4a6b9bf to c83fd1f Compare April 17, 2023 16:17

aravindhp commented Apr 17, 2023

View reviewed changes

aravindhp force-pushed the OCPBUGS-11652 branch from c83fd1f to 1789830 Compare April 17, 2023 17:16

ardaguclu reviewed Apr 18, 2023

View reviewed changes

aravindhp force-pushed the OCPBUGS-11652 branch from 1789830 to a1daedc Compare April 18, 2023 19:35

aravindhp commented Apr 18, 2023

View reviewed changes

aravindhp marked this pull request as ready for review April 24, 2023 18:33

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 24, 2023

openshift-ci Bot requested a review from ardaguclu April 24, 2023 18:34

ardaguclu mentioned this pull request Apr 25, 2023

WIP: Enable oc adm node-logs test again openshift/origin#27893

Closed

soltysh mentioned this pull request Apr 26, 2023

WIP: oc adm node-logs: Add compatibility with NodeLogQuery feature #1392

Closed

aravindhp force-pushed the OCPBUGS-11652 branch from a1daedc to 6350165 Compare April 27, 2023 23:28

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 9, 2023

soltysh suggested changes May 9, 2023

View reviewed changes

Comment thread pkg/cli/admin/node/logs.go

Comment thread pkg/cli/admin/node/logs.go Outdated

Comment thread pkg/cli/admin/node/logs.go Outdated

openshift-ci Bot assigned soltysh May 9, 2023

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label May 9, 2023

aravindhp commented May 9, 2023

View reviewed changes

Comment thread pkg/cli/admin/node/logs.go

Comment thread pkg/cli/admin/node/logs.go Outdated

Comment thread pkg/cli/admin/node/logs.go Outdated

aravindhp force-pushed the OCPBUGS-11652 branch from 6350165 to 0967cf6 Compare May 9, 2023 22:01

ardaguclu reviewed May 10, 2023

View reviewed changes

Comment thread pkg/cli/admin/node/logs.go

Comment thread pkg/cli/admin/node/logs.go

zhouying7780 reviewed May 17, 2023

View reviewed changes

OCPBUGS-11652: Extend adm node-logs to new API

a99b3f0

- Map --tail internally to tailLines - Map --unit internally to query - Use service instead of journal in the help to normalize across Linux and Windows

aravindhp force-pushed the OCPBUGS-11652 branch from 0967cf6 to a99b3f0 Compare May 17, 2023 16:48

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2023

soltysh approved these changes Jun 5, 2023

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 5, 2023

openshift-merge-robot merged commit 7339186 into openshift:master Jun 5, 2023

aravindhp deleted the OCPBUGS-11652 branch July 21, 2023 23:15

	return fmt.Errorf("cannot combine --query with --unit or --path")
	return fmt.Errorf("--path and --unit flags are mutually exclusive with --query")

Conversation

aravindhp commented Apr 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Apr 14, 2023

Uh oh!

openshift-ci-robot commented Apr 14, 2023

Uh oh!

aravindhp commented Apr 14, 2023

Uh oh!

aravindhp commented Apr 14, 2023

Uh oh!

aravindhp commented Apr 14, 2023

Uh oh!

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

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

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!

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

ardaguclu commented Apr 19, 2023

Uh oh!

aravindhp commented Apr 24, 2023

Uh oh!

aravindhp commented Apr 24, 2023

Uh oh!

ardaguclu commented Apr 28, 2023

Uh oh!

ardaguclu commented May 9, 2023

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aravindhp commented May 9, 2023

Uh oh!

aravindhp commented Apr 14, 2023 •

edited

Loading