[OCPNODE-553] Migrate ICSP to ImageDigest,TagMirrorSet CRDs #15

QiWang19 · 2022-04-05T03:50:41Z

Migrate ICSP to ImageDigest,TagMirrorSet.
MCO has handled the object conversoin from icsp to ImageDiegstMirrorSet objects.

Epic: https://issues.redhat.com/browse/OCPNODE-521
Epic: https://issues.redhat.com/browse/OCPNODE-810

Signed-off-by: Qi Wang [email protected]

mtrmac

Just an extremely brief skim for now, not really a review.

Let’s start with the ICSP discussion.

mtrmac · 2022-04-05T12:36:57Z

pkg/registries/registries.go

+// mergedDigestMirrorSets processes idmsRules and returns a set of ImageDigestMirrors, one for each Source value,
+// ordered consistently with the preference order of the individual entries (if possible)
+// E.g. given mirror sets (B, C) and (A, B), it will combine them into a single (A, B, C) set.
+func mergedDigestMirrorSets(idmsRules []*apicfgv1.ImageDigestMirrorSet) ([]apicfgv1.ImageDigestMirrors, error) {


At a first glance it seems to me that it should be possible to share more of the logic, but I haven’t read the code that carefully. (I do appreciate there’s a topoSortRepos now.)

Maybe an object that maintains disjointSets/mirrorBlockSource, with methods very vaguely like addMirrorSet(source, neverContactSource, mirrors) and mergedMirrorSets() []struct{source, neverContactSource, mirrors}? That way the two functions here would deal only with the data conversion with no logic at all.

Or maybe, instead of mergedMirrorSets, a sourceList() []string and updateRegistry(source, *Registry), to share more of the code in EditRegistriesConfig that just consumes the data from mergedMirrorSets.

pkg/registries/registries.go

QiWang19 · 2022-04-06T03:28:25Z

@mtrmac PTAL. More code got shared.

nee1esh · 2022-04-06T04:11:30Z

/retest

Update go version from 1.16 to 1.17. openshift/runtime-utils#15 CI failed since the go is out of date, update the go version to fix it. Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2022-04-06T17:01:20Z

/retest

openshift-bot · 2022-07-08T01:05:22Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-ci · 2022-07-08T01:05:31Z

@QiWang19: PR needs rebase.

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-bot · 2022-08-07T01:32:02Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

QiWang19 · 2022-08-17T14:24:55Z

@mtrmac could you review it? ICSP object migration should be done in MCO, this PR update uses new APIs.

rphillips · 2022-08-17T18:33:38Z

/remove-lifecycle rotten

rphillips · 2022-08-17T18:46:11Z

The pointer indirection is likely not needed. Can we refactor this by removing the pointer indirection such that the functions do not take a pointer, like:

disjointSets: map[string]*[]*[]apicfgv1.ImageMirror{},

and

func (sets *mirrorSets) addMirrorSets(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors *[]apicfgv1.ImageMirror)

The mirrors attribute probably doesn't need to be a pointer. Likewise for the other functions in the file.

mtrmac · 2022-08-18T23:39:18Z

@mtrmac could you review it? ICSP object migration should be done in MCO, this PR update uses new APIs.

Is that viable?

https://github.com/openshift/enhancements/pull/873/files still says the v1alpha1 API will remain. So external users are going to be creating them over the lifetime of the cluster.
At the very least https://github.com/mtrmac/openshift-apiserver/blob/master/pkg/image/apiserver/registry/imagestreamimport/rest.go#L217-L221 calls EditRegistries (and per the IDMS/ITMS enhancement, IIRC quite a few more repositories need to do so in the future). So is the idea that MCO is going to immediately convert, and delete, every created ICSP , and API consumers can then only deal with the new types? That seems fine for the controllers…
… but what about users? They are going to create a ICSP and just not see it in the API after a few seconds. Is that reasonable?

(It might well be fine. It’s just not what I vaguely remember the earlier migration conversations to be like, and I just thought I’d ask before we commit to that direction.)

mtrmac · 2022-08-19T00:55:22Z

So is the idea that MCO is going to immediately convert, and delete, every created ICSP , and API consumers can then only deal with the new types?

I’m sorry, actually reading the code, MCO is keeping the ICSP resources around, so they are still available to old consumers.

That seems perfect to me. Back to actually reading this PR…

Upgrade to go 1.18, openshift/runtime-utils#15 failed the ci since go is out of date. Signed-off-by: Qi Wang <[email protected]>

mtrmac

Two API semantics questions

And I’d like to push a bit further on the code sharing: The need to propagate blocked/insecure to sub-scopes is somewhat non-obvious, and having that triplicated feels a bit risky to me.

mtrmac · 2022-09-01T20:01:41Z

pkg/registries/registries.go

-	for _, mirror := range set.Mirrors {
-		if mirror != set.Source {
+// rdmContainsARealMirror returns true if mirrors contains at least one entry that is not source.
+func rdmContainsARealMirror(source string, mirrors []apicfgv1.ImageMirror) bool {


rdm refers to RepositoryDigestMirrors, so this should probably be renamed. mirrorsContainARealMirror? mirrorsContainSomeOtherThan?

mtrmac · 2022-09-01T20:05:26Z

pkg/registries/registries.go

+	}
+}
+
+// sourceList collects the mirror sources and sort them in increasing order


Suggested change

// sourceList collects the mirror sources and sort them in increasing order

// sourceList collects the mirror sources and returns them sorted in increasing order

to be a bit clearer that this this does not affect the mirrorSets object.

mtrmac · 2022-09-01T20:07:09Z

pkg/registries/registries.go

+
+func (sets *mirrorSets) addMirrorSets(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {
+	if !rdmContainsARealMirror(source, mirrors) {
+		return // No mirrors (or mirrors that only repeat the authoritative source) is not really a mirror set.


Is it possible to have such a no-mirror object with NeverContactSource? If so, should we still set mirrorBlockSource?

(I’m most tempted to say that such an object is invalid and should be rejected, I’m not sure if it is practical. If it isn’t, both setting mirrorBlockSource and the current approach is, I guess, acceptable, but with the current approach, a extending the comment that yes, we ignore mirrorSourcePolicy and that’s intentional, would be useful.)

mtrmac · 2022-09-01T20:25:41Z

pkg/registries/registries.go

+	sources := tagMirrorSets.sourceList()
+	// Convert the sets of mirrors
+	res := []apicfgv1.ImageTagMirrors{}
+	for _, source := range sources {
+		source, mirrors, err := tagMirrorSets.mergedMirrors(source)
+		if err != nil {
+			return nil, err
+		}
+		imageTagMirror := apicfgv1.ImageTagMirrors{
+			Source:  source,
+			Mirrors: mirrors,
+		}
+		if tagMirrorSets.mirrorBlockSource[source] {
+			imageTagMirror.MirrorSourcePolicy = apicfgv1.NeverContactSource
+		}
+		res = append(res, imageTagMirror)
 	}
-	sort.Strings(sources)
+	return res, nil


I don’t think we need to return correctly-typed Image{Tag,Digest}Mirrors from the pair of these functions — so we could define a local type mergedMirrorsItem struct { source, mirrors, mirrorSourcePolicy } and all of this loop could be also shared.

mtrmac · 2022-09-01T20:32:22Z

pkg/registries/registries.go

 	return res, nil
 }

+func topoSortRepos(source string, ds *[][]apicfgv1.ImageMirror) ([]string, error) {


This is not just topological sort. Maybe {shared,raw}MergedMirrors? (Or, alternatively, if the loop calling this can be made shared, it can be inlined back. OTOH having this code as a separate function might be a better granularity for unit testing.)

mtrmac · 2022-09-01T20:58:04Z

pkg/registries/registries_test.go

-			set := apioperatorsv1alpha1.RepositoryDigestMirrors{
-				Source:  source,
-				Mirrors: tt.mirrors,
+			set := &apicfgv1.ImageTagMirrorSet{


Non-blocking: If set is not used other than to read one of its fields, it doesn’t need to be constructed.

pkg/registries/registries_test.go

mtrmac · 2022-09-01T21:05:19Z

pkg/registries/registries_test.go

+			_, err := toml.Decode(string(templateBytes), &config)
+			require.NoError(t, err)
+			err = EditRegistriesConfig(&config, tt.insecure, tt.blocked, tt.icspRules, tt.idmsRules, tt.itmsRules)
+			require.NotNil(t, err)


Absolutely non-blocking: assert.NoError would be a better match here.

mtrmac · 2022-09-01T21:08:58Z

pkg/registries/registries_test.go

+		idmsRules         []*apicfgv1.ImageDigestMirrorSet
+		itmsRules         []*apicfgv1.ImageTagMirrorSet
+		icspRules         []*apioperatorsv1alpha1.ImageContentSourcePolicy
+		want              sysregistriesv2.V2RegistriesConf


insecure, blocked, want are unused, so that can be dropped.

Non-blocking: This test could be shorter and somewhat easier to fit on the screen with:

Only including one set of mirrors instead of three in every item.

Making the test data idmsNonempty, itmsNonempty, icspNonempty bool, and letting the testing loop provide data or an empty array.

mtrmac · 2022-09-01T21:11:32Z

pkg/registries/registries_test.go

+					Spec: apicfgv1.ImageTagMirrorSetSpec{
+						ImageTagMirrors: []apicfgv1.ImageTagMirrors{ // other.com is neither insecure nor blocked
+							{Source: "insecure.com/ns-i1", Mirrors: []apicfgv1.ImageMirror{"blocked.com/ns-b1", "other.com/ns-o1"}},
+							{Source: "blocked.com/ns-b/ns2-b", Mirrors: []apicfgv1.ImageMirror{"other.com/ns-o2", "insecure.com/ns-i2"}},
+							{Source: "other.com/ns-o3", Mirrors: []apicfgv1.ImageMirror{"insecure.com/ns-i2", "blocked.com/ns-b/ns3-b", "foo.insecure-example.com/bar"}},
+						},


This test does not set anything to insecure nor blocked, so using the insecure.com/blocked.com domains is a bit unexpected.

What is being tested here? Just a smoke-test of correctly creating tag entries? Two entries would be enough for that.

mtrmac · 2022-09-01T21:33:48Z

The failing tests do seem to suggest that updating to Go ≥ 1.18 is required to be able to consume the new API module.

mtrmac · 2022-09-01T21:35:30Z

The failing tests do seem to suggest that updating to Go ≥ 1.18 is required to be able to consume the new API module.

Ah, that’s being handled in openshift/release#31916 .

Upgrade to go 1.18, openshift/runtime-utils#15 failed the ci since go is out of date. Signed-off-by: Qi Wang <[email protected]> Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2022-09-03T16:59:34Z

@mtrmac PTAL

mtrmac

Thanks!

Implementation LGTM, just a few cleanup suggestions. Looking at the tests, I’m unsure why various tests were just dropped.

mtrmac · 2022-09-05T17:36:51Z

pkg/registries/registries.go


-	// Sort the sets of mirrors by Source to ensure deterministic output
+// sourceList collects the mirror sources and returns them sorted in increasing order
+func (sets *mirrorSets) sourceList() []string {


This now has only one caller and it can be inlined back.

mtrmac · 2022-09-05T17:37:23Z

pkg/registries/registries.go

+	return sources
+}
+
+func (sets *mirrorSets) addMirrorSets(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {


Suggested change

func (sets *mirrorSets) addMirrorSets(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {

func (sets *mirrorSets) addMirrorSet(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {

sets is multiple sets, this is adding a single one.

mtrmac · 2022-09-05T17:38:48Z

pkg/registries/registries.go

+	strMirrors := []string{}
+	for _, m := range mirrors {
+		strMirrors = append(strMirrors, (string(m)))
+	}


Absolutely non-blocking: This copy is probably avoidable, perhaps with some more casts in the merging implementation.

(It’s a bit weird for ICSP to do a copy to convert to ImageMirror, only for this code to convert back to strings.)

But the cost is trivial either way, I’m not worried about it at all.

mtrmac · 2022-09-05T17:39:51Z

pkg/registries/registries.go

+}
+
+// mergedMirrors generates deterministic order of mirrors for a given source
+func (sets *mirrorSets) mergedMirrors(source string) (string, []string, error) {


This doesn’t need to return source, it’s just the caller-supplied value.

mtrmac · 2022-09-05T17:41:21Z

pkg/registries/registries.go

+	return source, sortedRepos, nil
+}
+
+type mergedMirrorsItem struct {


Reading the full context, (contrary to my earlier suggestion — my mistake) mergedMirrorSet is probably the right name. That’s what it is (unlike “item”, which says nothing), and both functions like mergedICSPMirrorSets and variables like tagMirrorSets use that naming.

mtrmac · 2022-09-05T17:50:25Z

pkg/registries/registries_test.go

+				})
+			}
+			tcRes := tc.result
+			if tc.name == "Separate mirror sets" {


Please do this in the data.

mtrmac · 2022-09-05T17:51:30Z

pkg/registries/registries_test.go

+			if err != nil {
+				t.Errorf("Error %v", err)
+				return
+			}


Absolutely non-blocking: require.NoError(t, err) is shorter. This works just fine — and I realize the original was this way as well.

mtrmac · 2022-09-05T17:54:13Z

pkg/registries/registries_test.go

+			idmsRules: []*apicfgv1.ImageDigestMirrorSet{
+				{
+					Spec: apicfgv1.ImageDigestMirrorSetSpec{
+						ImageDigestMirrors: []apicfgv1.ImageDigestMirrors{ // other.com is neither insecure nor blocked


This comment seems inapplicable.

mtrmac · 2022-09-05T17:54:18Z

pkg/registries/registries_test.go

+			itmsRules: []*apicfgv1.ImageTagMirrorSet{
+				{
+					Spec: apicfgv1.ImageTagMirrorSetSpec{
+						ImageTagMirrors: []apicfgv1.ImageTagMirrors{ // other.com is neither insecure nor blocked


This comment seems inapplicable.

mtrmac · 2022-09-05T17:57:03Z

pkg/registries/registries_test.go

-				{Source: "source.example.net", Mirrors: []string{"z1.example.net", "y2.example.net", "x3.example.net"}},
-			},
-		},
-		{


The following test cases were dropped. Is there a specific reason for that? If not, please add them back.

Added back. Accidently removed during the last code cleanup.

QiWang19 · 2022-09-07T00:17:39Z

@mtrmac PTAL.

mtrmac

One very last thing - the tc.name == … condition.

LGTM otherwise.

mtrmac · 2022-09-23T17:24:14Z

pkg/registries/registries.go

+		}
+		sourceInGraph := false
+		for _, m := range mirrors {
+			if string(m) == source {


Non-blocking: The string() cast can now be dropped.

mtrmac · 2022-09-23T17:27:40Z

pkg/registries/registries.go

+		for i := range itms.Spec.ImageTagMirrors {
+			set := itms.Spec.ImageTagMirrors[i]


Absolutely non-blocking:

for _, set := range … { /* use set */ }

would be one variable and one line shorter. (In all three instances.)

mtrmac · 2022-09-23T17:32:37Z

pkg/registries/registries.go

 				reg.Mirrors = updated
 			}
 		}
+


(Absolutely non-blocking: drop this line? I don’t think it separates “paragraphs” / individual ideas when placed here.)

mtrmac · 2022-09-23T17:49:14Z

pkg/registries/registries_test.go

+func TestMergedICSPMirrorSets(t *testing.T) {
+	for _, tc := range mergedMirrorsetsTestcases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.name == "Separate mirror sets with mirrorSourcePolicy set" {


Please make this driven by a real data field.

Either here,

if tc.requiresMirrorSourcePolicy { t.Skip() }

(and set the tc.requiresMirrorSourcePolicy field on the test case), or (preferably?) make this fully automatic below, with something like

if item.mirrorSourcePolicy != "" { t.Skip("…") }

Add support for ImageDigestMirrorSet, ImageTagMirrorSet CRD Epic: https://issues.redhat.com/browse/OCPNODE-521 Epic: https://issues.redhat.com/browse/OCPNODE-810 Signed-off-by: Qi Wang <[email protected]>

Signed-off-by: Qi Wang <[email protected]>

openshift-ci · 2022-09-26T18:29:25Z

@QiWang19: 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.

mtrmac · 2022-09-26T19:02:42Z

/approve

mtrmac · 2022-09-26T19:05:04Z

/lgtm

openshift-ci · 2022-09-26T19:05:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, QiWang19

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

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

mtrmac · 2022-09-26T19:17:46Z

Thanks!

openshift-ci bot requested review from bparees and mtrmac April 5, 2022 03:51

QiWang19 mentioned this pull request Apr 5, 2022

[OCPNODE-553] Add CRD ImageDigestMirrorSet,ImageTagMirrorSet openshift/machine-config-operator#3037

Merged

mtrmac reviewed Apr 5, 2022

View reviewed changes

mtrmac mentioned this pull request Apr 5, 2022

Bug 1823143: wire ICSP lookups to oc image info openshift/oc#829

Merged

QiWang19 mentioned this pull request Apr 5, 2022

Update go version of runtime-utils ci config openshift/release#27581

Merged

QiWang19 force-pushed the mirrorset-crd branch 4 times, most recently from e44218b to 1226049 Compare April 6, 2022 00:05

QiWang19 force-pushed the mirrorset-crd branch from 1226049 to 8ecf024 Compare April 6, 2022 17:09

QiWang19 changed the title ~~[OCPNODE-553] Add support ImageDigest,TagMirrorSet CRDs~~ [OCPNODE-553] [WIP] Add support ImageDigest,TagMirrorSet CRDs Apr 8, 2022

openshift-ci bot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 8, 2022

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 7, 2022

QiWang19 force-pushed the mirrorset-crd branch from 7017f07 to 268252f Compare August 16, 2022 04:58

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2022

QiWang19 changed the title ~~[OCPNODE-553] [WIP] Add support ImageDigest,TagMirrorSet CRDs~~ [OCPNODE-553] Migrate ICSP to ImageDigest,TagMirrorSet CRDs Aug 17, 2022

openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Aug 17, 2022

QiWang19 force-pushed the mirrorset-crd branch 2 times, most recently from 2e6d143 to bb15d8e Compare August 31, 2022 20:26

QiWang19 mentioned this pull request Sep 1, 2022

Update go version of runtime-utils ci openshift/release#31916

Merged

QiWang19 added a commit to QiWang19/release that referenced this pull request Sep 1, 2022

Update go 1.18 of runtime-utils ci

7d540a3

Upgrade to go 1.18, openshift/runtime-utils#15 failed the ci since go is out of date. Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the mirrorset-crd branch from bb15d8e to b20a8ee Compare September 1, 2022 20:12

mtrmac reviewed Sep 1, 2022

View reviewed changes

mtrmac reviewed Sep 5, 2022

View reviewed changes

QiWang19 force-pushed the mirrorset-crd branch 2 times, most recently from eca703f to c29fe4c Compare September 7, 2022 00:05

mtrmac reviewed Sep 23, 2022

View reviewed changes

QiWang19 added 2 commits September 26, 2022 14:20

Add support ImageDigest,TagMirrorSet CRDs

bb75088

Add support for ImageDigestMirrorSet, ImageTagMirrorSet CRD Epic: https://issues.redhat.com/browse/OCPNODE-521 Epic: https://issues.redhat.com/browse/OCPNODE-810 Signed-off-by: Qi Wang <[email protected]>

bump c/image

f2400d4

Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the mirrorset-crd branch from c29fe4c to f2400d4 Compare September 26, 2022 18:20

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

openshift-ci bot assigned mtrmac Sep 26, 2022

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

openshift-merge-robot merged commit 5c488b2 into openshift:master Sep 26, 2022

	// sourceList collects the mirror sources and sort them in increasing order
	// sourceList collects the mirror sources and returns them sorted in increasing order

	func (sets *mirrorSets) addMirrorSets(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {
	func (sets *mirrorSets) addMirrorSet(source string, mirrorSourcePolicy apicfgv1.MirrorSourcePolicy, mirrors []apicfgv1.ImageMirror) {

		for i := range itms.Spec.ImageTagMirrors {
		set := itms.Spec.ImageTagMirrors[i]

[OCPNODE-553] Migrate ICSP to ImageDigest,TagMirrorSet CRDs #15

[OCPNODE-553] Migrate ICSP to ImageDigest,TagMirrorSet CRDs #15

Uh oh!

Conversation

QiWang19 commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

QiWang19 commented Apr 6, 2022

Uh oh!

nee1esh commented Apr 6, 2022

Uh oh!

QiWang19 commented Apr 6, 2022

Uh oh!

openshift-bot commented Jul 8, 2022

Uh oh!

openshift-ci bot commented Jul 8, 2022

Uh oh!

openshift-bot commented Aug 7, 2022

Uh oh!

QiWang19 commented Aug 17, 2022

Uh oh!

rphillips commented Aug 17, 2022

Uh oh!

rphillips commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Aug 18, 2022

Uh oh!

mtrmac commented Aug 19, 2022

Uh oh!

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

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!

mtrmac commented Sep 1, 2022

Uh oh!

mtrmac commented Sep 1, 2022

Uh oh!

QiWang19 commented Sep 3, 2022

Uh oh!

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

Choose a reason for hiding this comment

QiWang19 commented Apr 5, 2022 •

edited

Loading

rphillips commented Aug 17, 2022 •

edited

Loading