Remove context field from structs #1290

slashpai · 2021-07-19T11:24:44Z

Following the discussion in slack we need to remove context from struct field. Removing the context from struct field and passing it as first argument to functions that require it

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

slashpai · 2021-07-19T17:23:45Z

/retest

slashpai · 2021-07-20T03:45:39Z

@fpetkovski Could you please review?

simonpasquier · 2021-07-20T08:20:55Z

cmd/operator/main.go

@@ -203,7 +203,7 @@ func Main() int {

 	wg, ctx := errgroup.WithContext(ctx)

-	wg.Go(func() error { return o.Run(ctx.Done()) })
+	wg.Go(func() error { return o.Run(ctx, ctx.Done()) })


I would rather change the signature of Run() since it could use ctx.Done()

Suggested change

wg.Go(func() error { return o.Run(ctx, ctx.Done()) })

wg.Go(func() error { return o.Run(ctx) })

Is operator.Run trying to satisfy an interface that has Run(ctx, stopCh <-struct{})?

simonpasquier · 2021-07-20T08:29:29Z

test/e2e/framework/framework.go

@@ -143,7 +142,7 @@ func New(kubeConfigPath string) (*Framework, cleanUpFunc, error) {
 	// Prometheus client depends on setup above.
 	f.ThanosQuerierClient, err = NewPrometheusClientFromRoute(
 		openshiftRouteClient,
-		f.Ctx,
+		ctx,


(nit) to follow the conventions, ctx should be the first argument.

sthaha · 2021-07-20T23:48:49Z

pkg/client/status_reporter.go

 	namespace             string
 	userWorkloadNamespace string
 	version               string
 }

-func NewStatusReporter(ctx context.Context, client clientv1.ClusterOperatorInterface, name string, namespace, userWorkloadNamespace, version string) *StatusReporter {
+func NewStatusReporter(client clientv1.ClusterOperatorInterface, name string, namespace, userWorkloadNamespace, version string) *StatusReporter {


nit: name string, -> name

sthaha · 2021-07-20T23:59:47Z

pkg/operator/operator.go

-func (o *Operator) worker() {
-	for o.processNextWorkItem() {
+func (o *Operator) worker(ctx context.Context) {
+	for o.processNextWorkItem(ctx) {


@simonpasquier I am guessing that if the context gets cancelled, the next work item be processed. Could you please confirm my hunch?

good catch! yes I agree that we should verify whether the context has been cancelled before processing the next item.

at the same time, Run() currently doesn't wait for any of the goroutines to stop before exiting so it would need to be addressed not only for worker() IMHO. I'm ok with having a follow-up PR to "fix" this.

sthaha

The changes looks good to me but I think we need a test that validates the expected behaviour if a context is cancelled
WDYT @slashpai @simonpasquier ?

slashpai · 2021-07-21T11:51:53Z

pkg/client/status_reporter.go

+	unavailableMessage          string = "Rollout of the monitoring stack failed and is degraded. Please investigate the degraded status error."
+	asExpectedReason            string = "AsExpected"
+	StorageNotConfiguredMessage        = "Prometheus is running without persistent storage which can lead to data loss during upgrades and cluster disruptions. Please refer to the official documentation to see how to configure storage for Prometheus: https://docs.openshift.com/container-platform/4.8/monitoring/configuring-the-monitoring-stack.html"
+	StorageNotConfiguredReason         = "PrometheusDataPersistenceNotConfigured"


This was changed when ran go fmt

simonpasquier · 2021-07-22T15:41:05Z

The changes looks good to me but I think we need a test that validates the expected behaviour if a context is cancelled

I couldn't think of an easy way to test this so I'm fine without it.

simonpasquier · 2021-07-22T15:41:22Z

@slashpai could you rebase?

slashpai · 2021-07-22T15:43:33Z

@slashpai could you rebase?

Sure I will get that done when login tomorrow. Are we good with the changes or anymore changes are needed?

slashpai · 2021-07-23T04:57:00Z

@simonpasquier rebased

fpetkovski · 2021-07-23T05:34:07Z

/lgtm

fpetkovski · 2021-07-23T05:34:22Z

/hold for @simonpasquier's final review

fpetkovski · 2021-07-23T09:29:19Z

/lgtm

simonpasquier · 2021-07-23T10:20:40Z

/lgtm

openshift-ci · 2021-07-23T10:20:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fpetkovski, simonpasquier, slashpai

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~~ [fpetkovski,simonpasquier,slashpai]

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

slashpai · 2021-07-23T13:23:36Z

/retest

simonpasquier · 2021-07-23T14:03:55Z

/hold cancel

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 Jul 19, 2021

openshift-ci bot requested review from fpetkovski and prashbnair July 19, 2021 11:24

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

slashpai changed the title ~~WIP: Remove context field from structs~~ Remove context field from structs Jul 20, 2021

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 Jul 20, 2021

simonpasquier reviewed Jul 20, 2021

View reviewed changes

sthaha reviewed Jul 20, 2021

View reviewed changes

sthaha reviewed Jul 21, 2021

View reviewed changes

slashpai force-pushed the remove_ctx_struct branch from 43cb6ac to 09ea227 Compare July 21, 2021 07:18

slashpai commented Jul 21, 2021

View reviewed changes

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021

slashpai force-pushed the remove_ctx_struct branch from 091baa8 to 1fee8df Compare July 23, 2021 04:55

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2021

openshift-ci bot assigned fpetkovski Jul 23, 2021

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

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 23, 2021

slashpai added 3 commits July 23, 2021 13:27

pkg/client/status_reporter*.go: Remove ctx from statusreporter struct

272b96f

pkg/client/client*.go: Remove context from client struct

6e31878

operator: Remove context from operator struct

18fd4da

slashpai added 7 commits July 23, 2021 13:38

pkg/tasks: propagate real context

7d0b6e4

pkg: Update task func signature

236c87f

test/e2e: Remove context from framework struct

d073559

Fix go formatting

56d5668

Fix end to end tests

ea3a47a

Fix nits and address review comments

6fdbec1

pkg/tasks: Fix method calls

0978718

slashpai force-pushed the remove_ctx_struct branch from 5a60e3f to 0978718 Compare July 23, 2021 08:17

openshift-ci bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 23, 2021

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

test/e2e/thanos_ruler_test.go: Fix method call

d3bc73d

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

openshift-ci bot assigned simonpasquier Jul 23, 2021

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

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 23, 2021

openshift-merge-robot merged commit e302364 into openshift:master Jul 23, 2021

slashpai deleted the remove_ctx_struct branch October 13, 2022 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove context field from structs #1290

Remove context field from structs #1290

slashpai commented Jul 19, 2021

slashpai commented Jul 19, 2021

slashpai commented Jul 20, 2021

simonpasquier Jul 20, 2021

sthaha Jul 20, 2021

simonpasquier Jul 20, 2021

sthaha Jul 20, 2021

sthaha Jul 20, 2021

simonpasquier Jul 21, 2021

simonpasquier Jul 22, 2021

sthaha left a comment •

edited

slashpai Jul 21, 2021

simonpasquier commented Jul 22, 2021

simonpasquier commented Jul 22, 2021

slashpai commented Jul 22, 2021

slashpai commented Jul 23, 2021

fpetkovski commented Jul 23, 2021

fpetkovski commented Jul 23, 2021

fpetkovski commented Jul 23, 2021

simonpasquier commented Jul 23, 2021

openshift-ci bot commented Jul 23, 2021

slashpai commented Jul 23, 2021

simonpasquier commented Jul 23, 2021

	wg.Go(func() error { return o.Run(ctx, ctx.Done()) })
	wg.Go(func() error { return o.Run(ctx) })

Remove context field from structs #1290

Remove context field from structs #1290

Conversation

slashpai commented Jul 19, 2021

slashpai commented Jul 19, 2021

slashpai commented Jul 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthaha left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonpasquier commented Jul 22, 2021

simonpasquier commented Jul 22, 2021

slashpai commented Jul 22, 2021

slashpai commented Jul 23, 2021

fpetkovski commented Jul 23, 2021

fpetkovski commented Jul 23, 2021

fpetkovski commented Jul 23, 2021

simonpasquier commented Jul 23, 2021

openshift-ci bot commented Jul 23, 2021

slashpai commented Jul 23, 2021

simonpasquier commented Jul 23, 2021

sthaha left a comment •

edited