RFE-3765 - Allow Ingress to Modify the HAProxy Log Length when using a Sidecar #900

dafsjr · 2023-03-19T15:13:21Z

A code to implement the RFE-3765, I also modified the maxLength of the Syslog section to be the same of the Container section.

openshift-ci-robot · 2023-03-19T15:13:24Z

@dafsjr: This pull request references RFE-3765 which is a valid jira issue.

Details

In response to this:

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 · 2023-03-19T15:13:55Z

Hi @dafsjr. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

openshift-ci-robot · 2023-03-19T16:32:36Z

@dafsjr: This pull request references RFE-3765 which is a valid jira issue.

Details

In response to this:

A code to implement the RFE-3765, I also modified the maxLength of the Syslog section to be the same of the Container section.

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 · 2023-03-19T16:32:38Z

@dafsjr: This pull request references RFE-3765 which is a valid jira issue.

Details

In response to this:

A code to implement the RFE-3765, I also modified the maxLength of the Syslog section to be the same of the Container section.

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.

candita · 2023-04-05T15:40:42Z

@dafsjr Thank you for your contribution. This PR requires a change in openshift/api, not just edits in the vendor files. Please make the change in the API and @candita me when you have completed it.

openshift-ci-robot · 2023-04-11T03:12:53Z

@dafsjr: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

Details

In response to this:

A code to implement the RFE-3765, I also modified the maxLength of the Syslog section to be the same of the Container section.

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.

dafsjr · 2023-04-11T14:06:02Z

@candita if I understood correctly, it is done

candita · 2023-04-11T15:01:07Z

@candita if I understood correctly, it is done

@dafsjr I replied to your message in Slack with an example.

candita · 2023-04-11T15:03:26Z

Dockerfile

You should not need to change the Dockerfile, please revert.

sorry, I do not have access to the images in Dockerfile, I needed to use the origin image to compile.

candita · 2023-04-11T15:04:30Z

pkg/operator/controller/ingress/deployment.go

 	RouterSyslogFacilityEnvName  = "ROUTER_LOG_FACILITY"
 	RouterSyslogMaxLengthEnvName = "ROUTER_LOG_MAX_LENGTH"

+	RouterContainerMaxLengthEnvName = "ROUTER_LOG_MAX_LENGTH"


You cannot use the same user-visible value for two different environment variables.

OK, I tried something different.

candita · 2023-04-12T23:41:24Z

/assign

candita · 2023-04-24T17:49:38Z

/ok-to-test

candita · 2023-04-24T20:49:32Z

pkg/operator/controller/ingress/deployment_test.go

 			Destination: operatorv1.LoggingDestination{
-				Type:      operatorv1.ContainerLoggingDestinationType,
-				Container: &operatorv1.ContainerLoggingDestinationParameters{},
+				Type: operatorv1.ContainerLoggingDestinationType,


You can add this later, but this section should be to test the default, so don't make these changes here.

This comment is for L519, will repost there.

But it does apply to the addition of MaxLength in this section. This should only be the default, not a changed value in MaxLength.

ok, understood

This is setting up the default for testing. Don't add MaxLength here.

candita · 2023-04-26T14:18:01Z

pkg/operator/controller/ingress/deployment_test.go

 		{"ROUTER_CANONICAL_HOSTNAME", true, "router-" + ic.Name + "." + ic.Status.Domain},
 		{"ROUTER_LOG_FACILITY", false, ""},
-		{"ROUTER_LOG_MAX_LENGTH", false, ""},
+		{"ROUTER_LOG_MAX_LENGTH", true, "8192"},


See https://github.com/openshift/cluster-ingress-operator/pull/900/files/e965950e1d1bb700c455d019389a28104a75c964#r1175768966

E.g. don't do this here. Add a new test elsewhere, which tests the change from default to a new value.

See TestDeploymentConfigChanged for examples.

dafsjr · 2023-04-27T15:28:58Z

/retest-required

dafsjr · 2023-04-27T19:06:57Z

/test e2e-aws-ovn-single-node

dafsjr · 2023-04-28T18:23:53Z

/test e2e-gcp-operator

dafsjr · 2023-04-28T20:03:45Z

/test e2e-aws-ovn-single-node

dafsjr · 2023-07-19T12:52:25Z

/retest

dafsjr · 2023-07-19T17:57:55Z

/test e2e-aws-operator

candita · 2023-07-19T20:34:07Z

manifests/00-custom-resource-definition.yaml

                                maxLength:
                                  default: 1024
-                                  description: "maxLength is the maximum length of the syslog message \n If this field is empty, the maxLength is set to \"1024\"."
+                                  description: "maxLength is the maximum length of the log message. \n Valid values are integers in the range 480 to 4096, inclusive. \n When omitted, the default value is 1024."


@dafsjr why is the maximum different for syslog?

this file is the same of vendor/github.com/openshift/api/operator/v1/0000_50_ingress-operator_00-ingresscontroller.crd.yaml, it is autogenerated/updated, Miciah asked me to modify the message for syslog too in the PR for openshift/api.

But don't you want the same numeric maximum for syslog and sidecar log?

sorry, I understood a total different thing, I will reply in slack

I checked with @Miciah and this should be fine.

candita · 2023-07-19T20:47:06Z

test/e2e/operator_test.go

+		}).Stream(context.TODO())
+		if err != nil {
+			t.Errorf("failed to read logs from pod %s: %v", pod.Name, err)
+			return false, err


I think it would be ok to return false, nil here, which would indicate we want to try again until the timer expires. @Miciah or @frobware can you verify?

Yes, please log the error with t.Logf instead of t.Errorf, and then return false, nil so that the polling loop retries after logging the error.

candita · 2023-07-19T20:47:42Z

test/e2e/operator_test.go

+		data, err := ioutil.ReadAll(readCloser)
+		if err != nil {
+			t.Errorf("failed to read logs from pod %s: %v", pod.Name, err)
+			return false, err


Same as above, if I'm correct.

Suggested change

return false, err

return false, nil

candita · 2023-07-19T21:22:16Z

/lgtm

candita · 2023-07-19T21:43:04Z

test/e2e/operator_test.go

+		if err := readCloser.Close(); err != nil {
+			t.Errorf("failed to close logs reader for pod %s: %v", pod.Name, err)
+		}


As mentioned, please remove this and put in a defer function before starting scanner.Scan, here and in the other function you added.

candita · 2023-07-19T22:21:29Z

/lgtm
/approve

candita · 2023-07-19T22:25:37Z

/remove-hold

dafsjr · 2023-07-20T11:02:35Z

/retest

…n using a sidecar. * pkg/operator/controller/ingress/deployment.go: - Changed the name of the var RouterSyslogMaxLengthEnvName to RouterLogMaxLengthEnvName, thus the same var is used to Syslog and Container options - Added a validation for containerLoggingParameters, if not null, return all the parameters * pkg/operator/controller/ingress/rsyslog_configmap.go: - Added the parameter MaxMessageSize with value of 10k in the configMap generator for rsyslog.conf * test/e2e/all_test.go: - Add tests TestContainerLoggingMaxLength and TestContainerLoggingMinLength * test/e2e/operator_test.go: - Added the functions for the tests TestContainerLoggingMaxLength and TestContainerLoggingMinLength

openshift-ci · 2023-07-20T12:41:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: candita

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~~ [candita]

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

dafsjr · 2023-07-20T15:20:17Z

/retest

candita · 2023-07-20T15:59:11Z

/lgtm

openshift-ci · 2023-07-20T17:50:43Z

@dafsjr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ovn	`8184bce`	link	false	`/test e2e-gcp-ovn`

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 added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 19, 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 Mar 19, 2023

dafsjr marked this pull request as ready for review March 19, 2023 15:13

openshift-ci bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 19, 2023

openshift-ci bot requested review from lmzuccarelli and miheer March 19, 2023 15:14

dafsjr changed the title ~~RFE-3765: Allow Ingress to be modified the log length when using a sidecar~~ RFE-3765 - Allow Ingress to Modify the HAProxy Log Length when using a Sidecar Apr 11, 2023

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

candita reviewed Apr 11, 2023

View reviewed changes

openshift-ci bot assigned candita Apr 12, 2023

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2023

candita reviewed Apr 24, 2023

View reviewed changes

candita reviewed Apr 26, 2023

View reviewed changes

candita reviewed Jul 19, 2023

View reviewed changes

dafsjr force-pushed the rfe-3765 branch from 99dd310 to c70da4a Compare July 19, 2023 21:20

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023

candita reviewed Jul 19, 2023

View reviewed changes

dafsjr force-pushed the rfe-3765 branch from c70da4a to a3055b2 Compare July 19, 2023 21:44

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023

dafsjr force-pushed the rfe-3765 branch from a3055b2 to f2a9753 Compare July 19, 2023 22:12

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023

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

dafsjr force-pushed the rfe-3765 branch from f2a9753 to 8184bce Compare July 20, 2023 12:40

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 20, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2023

openshift-merge-robot merged commit b4b5f42 into openshift:master Jul 20, 2023

This was referenced Jul 20, 2023

NE-1269: Replace bindata using embed #905

Merged

NE-1244: Use permissions instead of the "Contributor" role in Azure CredentialsRequest #929

Merged

candita mentioned this pull request Aug 2, 2023

NE-1143: Adds logic to validate headers. openshift/library-go#1449

Merged

RFE-3765 - Allow Ingress to Modify the HAProxy Log Length when using a Sidecar #900

RFE-3765 - Allow Ingress to Modify the HAProxy Log Length when using a Sidecar #900

Uh oh!

Conversation

dafsjr commented Mar 19, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 19, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Mar 19, 2023

Uh oh!

openshift-ci-robot commented Mar 19, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 19, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

candita commented Apr 5, 2023

Uh oh!

openshift-ci-robot commented Apr 11, 2023

Uh oh!

dafsjr commented Apr 11, 2023

Uh oh!

candita commented Apr 11, 2023

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!

candita commented Apr 12, 2023

Uh oh!

candita commented Apr 24, 2023

Uh oh!

candita Apr 24, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

dafsjr commented Apr 27, 2023

Uh oh!

dafsjr commented Apr 27, 2023

Uh oh!

dafsjr commented Apr 28, 2023

Uh oh!

dafsjr commented Apr 28, 2023

Uh oh!

dafsjr commented Jul 19, 2023

Uh oh!

dafsjr commented Jul 19, 2023

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!

dafsjr commented Mar 19, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 19, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 19, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 19, 2023 •

edited by openshift-ci bot

Loading

candita Apr 24, 2023 •

edited

Loading