OTA-560: Improve developer-oriented docs #871

petr-muller · 2022-12-06T18:16:00Z

Update CONTRIBUTING.md
Provide more convenience Makefile targets
Cleanup codegen Makefile targets
Move developer-facing content from README.md to docs/dev/README.md

This is a companion PR to #869

I have omitted the Installing CVO and operators in cluster and Using CVO to render the release-image locally sections from the original README b/c I don't think these use-cases are too interesting when developing CVO. I have updated a section on building a custom release payload to build from an existing payload (instead if imagestream) and added a section that instructs how to inject such release payload to the cluster

petr-muller · 2022-12-06T22:15:21Z

/retest

wking · 2022-12-09T18:22:26Z

Makefile

 verify: verify-codegen
-.PHONY: update-codegen-crds update-codegen verify-codegen-crds verify-codegen verify
+.PHONY: update-codegen verify-codegen verify


nit: dup verify-codegen entries? Also, the pattern in the rest of the Makefile appears to be a new .PHONY entry after each target. I'm agnostic about whether we use that or a single large .PHONY entry, but I'd rather not split between both patterns.

I've just removed the codegen stuff entirely in a separate commit. The update-codegen was just a tombstone and we do not use it in our CI jobs: https://github.com/openshift/release/blob/master/ci-operator/config/openshift/cluster-version-operator/openshift-cluster-version-operator-master.yaml

docs/dev/README.md

- Update `CONTRIBUTING.md` - Provide more convenience `Makefile` targets - Cleanup codegen `Makefile` targets - Move developer-facing content from `README.md` to `docs/dev/README.md`

The `update-codegen.sh` script is now a NOP, so we can remove it together with the script that checks its output, related `Makefile` targets and references from other helper scripts

petr-muller · 2022-12-13T16:13:25Z

I'm not touching any code, so the unit test failure is very likely to be a flake, but it's a one that I've never seen before:

 --- FAIL: TestCache (0.54s)
    cache_test.go:48: 2022-12-13 15:55:27.095319872 +0000 UTC m=+0.007024386 round 0, condition 0 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.09587749 +0000 UTC m=+0.007582027 round 0, condition 1 -> false client-side throttling: only 515.923µs has elapsed since the last match call completed for this cluster condition backend; this cached cluster condition request has been queued for later execution
    cache_test.go:48: 2022-12-13 15:55:27.218866363 +0000 UTC m=+0.130570872 round 1, condition 0 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.218911953 +0000 UTC m=+0.130616467 round 1, condition 1 -> false client-side throttling: only 123.589973ms has elapsed since the last match call completed for this cluster condition backend; this cached cluster condition request has been queued for later execution
    cache_test.go:48: 2022-12-13 15:55:27.323690227 +0000 UTC m=+0.235394741 round 2, condition 0 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.323758035 +0000 UTC m=+0.235462555 round 2, condition 1 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.424694073 +0000 UTC m=+0.336398592 round 3, condition 0 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.424798062 +0000 UTC m=+0.336502582 round 3, condition 1 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.528678823 +0000 UTC m=+0.440383348 round 4, condition 0 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.528740741 +0000 UTC m=+0.440445271 round 4, condition 1 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.630991294 +0000 UTC m=+0.542695806 round 5, condition 0 -> true <nil>
    cache_test.go:48: 2022-12-13 15:55:27.631030803 +0000 UTC m=+0.542735309 round 5, condition 1 -> true <nil>
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xad513f]
goroutine 19 [running]:
testing.tRunner.func1.2({0xb64a00, 0x13d22c0})
	/usr/lib/golang/src/testing/testing.go:1396 +0x24e
testing.tRunner.func1()
	/usr/lib/golang/src/testing/testing.go:1399 +0x39f
panic({0xb64a00, 0x13d22c0})
	/usr/lib/golang/src/runtime/panic.go:884 +0x212
github.com/openshift/cluster-version-operator/pkg/clusterconditions/mock.(*Mock).Match(0xc000180a50, {0x0?, 0xc56bd7?}, 0x0)
	/go/src/github.com/openshift/cluster-version-operator/pkg/clusterconditions/mock/mock.go:68 +0x7f
github.com/openshift/cluster-version-operator/pkg/clusterconditions/cache.(*Cache).Match(0xc0001ddec8, {0xd70d30, 0xc0001ac000}, 0xc000012138)
	/go/src/github.com/openshift/cluster-version-operator/pkg/clusterconditions/cache/cache.go:133 +0x9a2
github.com/openshift/cluster-version-operator/pkg/clusterconditions/cache.TestCache(0xc000286000)
	/go/src/github.com/openshift/cluster-version-operator/pkg/clusterconditions/cache/cache_test.go:47 +0xa93
testing.tRunner(0xc000286000, 0xcbbf48)
	/usr/lib/golang/src/testing/testing.go:1446 +0x10b
created by testing.(*T).Run
	/usr/lib/golang/src/testing/testing.go:1493 +0x35f
FAIL	github.com/openshift/cluster-version-operator/pkg/clusterconditions/cache	0.587s

petr-muller · 2022-12-13T16:13:35Z

/test unit

petr-muller · 2022-12-13T16:16:25Z

I'm not ignoring the flake, I'm looking if I can fix it in a separate PR

petr-muller · 2022-12-13T16:28:59Z

/test e2e-agnostic-upgrade-into-change

Azure flakes during cluster installation:

 level=error msg=Error: creating/updating Load Balancer Backend Address Pool "Load Balancer Backend Address Pool: (Backend Address Pool Name \"ci-op-2psbn19r-b1493-qqklb\" / Load Balancer Name \"ci-op-2psbn19r-b1493-qqklb\" / Resource Group \"ci-op-2psbn19r-b1493-qqklb-rg\")": network.LoadBalancerBackendAddressPoolsClient#CreateOrUpdate: Failure sending request: StatusCode=500 -- Original Error: Code="MultipleErrorsOccurred" Message="Multiple error occurred: . Please see details." Details=[{"code":"InternalServerError","message":"Encountered internal server error. Diagnostic information: timestamp '20221213T161143Z', subscription id 'd38f1e38-4bed-438e-b227-833f997adf6a', tracking id 'a6b364d1-60d9-4263-892a-317b8fb28b8d', request correlation id '2e2361f4-15bd-b6bf-c55d-4b480921a88e'."},{"code":"InternalServerError","message":"Encountered internal server error. Diagnostic information: timestamp '20221213T161143Z', subscription id 'd38f1e38-4bed-438e-b227-833f997adf6a', tracking id 'a6b364d1-60d9-4263-892a-317b8fb28b8d', request correlation id '2e2361f4-15bd-b6bf-c55d-4b480921a88e'."}]

CONTRIBUTING.md

wking

/lgtm

openshift-ci · 2022-12-13T17:12:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: petr-muller, wking

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~~ [petr-muller,wking]

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

LalatenduMohanty · 2022-12-13T17:44:09Z

/hold

Looking at `Cache.Match()` implementation, the `c.Condition.Match()` on L133 can be called with `targetCondition=nil` and that seems to be intentional. In tests, `c.Condition` is a mock, which did not expect that pointer to be `nil` and dereferenced it, resulting in an occassional panic in tests like openshift#871 (comment). The fix stores the full pointer (its deepcopy to avoid sharing that memory) in the mock instead. I needed to restructure the tests a little to allow adding a nil parameter testcase

LalatenduMohanty · 2022-12-13T17:50:25Z

/hold cancel

Looking at `Cache.Match()` implementation, the `c.Condition.Match()` on L133 can be called with `targetCondition=nil` and that seems to be intentional. In tests, `c.Condition` is a mock, which did not expect that pointer to be `nil` and dereferenced it, resulting in an occassional panic in tests like openshift#871 (comment). The fix stores the full pointer (its deepcopy to avoid sharing that memory) in the mock instead. I needed to restructure the tests a little to allow adding a nil parameter testcase

openshift-ci-robot · 2022-12-13T18:14:32Z

/retest-required

Remaining retests: 0 against base HEAD fa80044 and 2 for PR HEAD b3049c1 in total

openshift-ci-robot · 2022-12-13T22:16:29Z

/retest-required

Remaining retests: 0 against base HEAD 72dba90 and 1 for PR HEAD b3049c1 in total

openshift-ci-robot · 2022-12-14T09:19:13Z

/retest-required

Remaining retests: 0 against base HEAD 56bcb24 and 0 for PR HEAD b3049c1 in total

petr-muller · 2022-12-14T14:56:57Z

/override ci/prow/e2e-agnostic-upgrade-into-change

this is a docs change

openshift-ci · 2022-12-14T14:59:05Z

@petr-muller: Overrode contexts on behalf of petr-muller: ci/prow/e2e-agnostic-upgrade-into-change

In response to this:

/override ci/prow/e2e-agnostic-upgrade-into-change

this is a docs 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.

openshift-ci · 2022-12-14T17:48:27Z

@petr-muller: The following tests 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-agnostic-upgrade-into-change	`b3049c1`	link	unknown	`/test e2e-agnostic-upgrade-into-change`
ci/prow/e2e-agnostic	`b3049c1`	link	unknown	`/test e2e-agnostic`
ci/prow/e2e-agnostic-upgrade-out-of-change	`b3049c1`	link	unknown	`/test e2e-agnostic-upgrade-out-of-change`

Full PR test history. Your PR dashboard.

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.

Looking at `Cache.Match()` implementation, the `c.Condition.Match()` on L133 can be called with `targetCondition=nil` and that seems to be intentional. In tests, `c.Condition` is a mock, which did not expect that pointer to be `nil` and dereferenced it, resulting in an occassional panic in tests like openshift#871 (comment). The fix stores the full pointer (its deepcopy to avoid sharing that memory) in the mock instead. I needed to restructure the tests a little to allow adding a nil parameter testcase

petr-muller mentioned this pull request Dec 6, 2022

OTA-560: Modernize README.md to be user-centered #869

Merged

openshift-ci bot requested review from jottofar and LalatenduMohanty December 6, 2022 18:17

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2022

petr-muller force-pushed the ota-560-developer-content branch 4 times, most recently from 6fa2a10 to dfb8f22 Compare December 6, 2022 18:25

petr-muller mentioned this pull request Dec 6, 2022

Build CVO from UBI8 instead of obsolete origin-v4.0:base #872

Merged

wking reviewed Dec 9, 2022

View reviewed changes

docs/dev/README.md Outdated Show resolved Hide resolved

petr-muller added 2 commits December 13, 2022 16:49

Improve developer-oriented docs

7971a6a

- Update `CONTRIBUTING.md` - Provide more convenience `Makefile` targets - Cleanup codegen `Makefile` targets - Move developer-facing content from `README.md` to `docs/dev/README.md`

Cleanup obsolete codegen helpers

b3049c1

The `update-codegen.sh` script is now a NOP, so we can remove it together with the script that checks its output, related `Makefile` targets and references from other helper scripts

petr-muller force-pushed the ota-560-developer-content branch from dfb8f22 to b3049c1 Compare December 13, 2022 15:49

LalatenduMohanty suggested changes Dec 13, 2022

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

openshift-ci bot assigned LalatenduMohanty Dec 13, 2022

wking approved these changes Dec 13, 2022

View reviewed changes

openshift-ci bot assigned wking Dec 13, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 13, 2022

petr-muller mentioned this pull request Dec 13, 2022

tests: fix rare nil dereference in TestCache #875

Merged

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

openshift-merge-robot merged commit 9c23665 into openshift:master Dec 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTA-560: Improve developer-oriented docs #871

OTA-560: Improve developer-oriented docs #871

petr-muller commented Dec 6, 2022

petr-muller commented Dec 6, 2022

wking Dec 9, 2022

petr-muller Dec 13, 2022 •

edited

petr-muller commented Dec 13, 2022

petr-muller commented Dec 13, 2022

petr-muller commented Dec 13, 2022

petr-muller commented Dec 13, 2022

wking left a comment

openshift-ci bot commented Dec 13, 2022

LalatenduMohanty commented Dec 13, 2022

LalatenduMohanty commented Dec 13, 2022

openshift-ci-robot commented Dec 13, 2022

openshift-ci-robot commented Dec 13, 2022

openshift-ci-robot commented Dec 14, 2022

petr-muller commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

OTA-560: Improve developer-oriented docs #871

OTA-560: Improve developer-oriented docs #871

Conversation

petr-muller commented Dec 6, 2022

petr-muller commented Dec 6, 2022

wking Dec 9, 2022

Choose a reason for hiding this comment

petr-muller Dec 13, 2022 • edited

Choose a reason for hiding this comment

petr-muller commented Dec 13, 2022

petr-muller commented Dec 13, 2022

petr-muller commented Dec 13, 2022

petr-muller commented Dec 13, 2022

wking left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 13, 2022

LalatenduMohanty commented Dec 13, 2022

LalatenduMohanty commented Dec 13, 2022

openshift-ci-robot commented Dec 13, 2022

openshift-ci-robot commented Dec 13, 2022

openshift-ci-robot commented Dec 14, 2022

petr-muller commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

openshift-ci bot commented Dec 14, 2022

petr-muller Dec 13, 2022 •

edited