From f64955d0e815170d182636bf6fb0095b62cbad02 Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Tue, 12 Dec 2023 16:58:59 +0100 Subject: [PATCH 1/8] fix, botreview, strategy: ignore unrelated files in kubevirtci-bump Previously all files were added as unwanted whenever a review was placed, even if there we no files related to kubevirtci-bump at all in the patch set. Now the review for kubevirtci-bump is only attached if there are relevant files in the PR. Signed-off-by: Daniel Hiller --- .../botreview/review/bump_kubevirtci.go | 30 +- .../botreview/review/bump_kubevirtci_test.go | 69 +++- external-plugins/botreview/review/review.go | 2 +- .../botreview/review/review_test.go | 68 +++- .../eviction-strategy-doc.patch0 | 13 + .../eviction-strategy-doc.patch1 | 127 ++++++ .../eviction-strategy-doc.patch2 | 20 + .../eviction-strategy-doc.patch3 | 13 + .../eviction-strategy-doc.patch4 | 13 + .../fix-containerdisks-migrations.patch00 | 94 +++++ .../fix-containerdisks-migrations.patch01 | 49 +++ .../fix-containerdisks-migrations.patch02 | 77 ++++ .../fix-containerdisks-migrations.patch03 | 18 + .../fix-containerdisks-migrations.patch04 | 22 ++ .../fix-containerdisks-migrations.patch05 | 19 + .../fix-containerdisks-migrations.patch06 | 371 ++++++++++++++++++ .../fix-containerdisks-migrations.patch07 | 219 +++++++++++ .../fix-containerdisks-migrations.patch08 | 26 ++ .../fix-containerdisks-migrations.patch09 | 137 +++++++ .../fix-containerdisks-migrations.patch10 | 70 ++++ .../fix-containerdisks-migrations.patch11 | 17 + .../fix-containerdisks-migrations.patch12 | 48 +++ .../fix-containerdisks-migrations.patch13 | 123 ++++++ .../fix-containerdisks-migrations.patch14 | 57 +++ .../fix-containerdisks-migrations.patch15 | 56 +++ .../fix-containerdisks-migrations.patch16 | 179 +++++++++ 26 files changed, 1909 insertions(+), 28 deletions(-) create mode 100644 external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16 diff --git a/external-plugins/botreview/review/bump_kubevirtci.go b/external-plugins/botreview/review/bump_kubevirtci.go index 74b7ade1ad..dea2c03135 100644 --- a/external-plugins/botreview/review/bump_kubevirtci.go +++ b/external-plugins/botreview/review/bump_kubevirtci.go @@ -46,7 +46,7 @@ func init() { type BumpKubevirtCI struct { relevantFileDiffs []*diff.FileDiff - unwantedFiles map[string][]*diff.Hunk + unwantedFiles map[string]struct{} } func (t *BumpKubevirtCI) IsRelevant() bool { @@ -56,25 +56,17 @@ func (t *BumpKubevirtCI) IsRelevant() bool { func (t *BumpKubevirtCI) AddIfRelevant(fileDiff *diff.FileDiff) { fileName := strings.TrimPrefix(fileDiff.NewName, "b/") - // store all hunks for unwanted files - if fileName != "cluster-up-sha.txt" && - fileName != "hack/config-default.sh" && - !strings.HasPrefix(fileName, "cluster-up/") { - for _, hunk := range fileDiff.Hunks { - if t.unwantedFiles == nil { - t.unwantedFiles = make(map[string][]*diff.Hunk, 0) - } - _, exists := t.unwantedFiles[fileName] - if !exists { - t.unwantedFiles[fileName] = []*diff.Hunk{hunk} - } else { - t.unwantedFiles[fileName] = append(t.unwantedFiles[fileName], hunk) - } - } + if fileName == "cluster-up-sha.txt" || + fileName == "hack/config-default.sh" || + strings.HasPrefix(fileName, "cluster-up/") { + t.relevantFileDiffs = append(t.relevantFileDiffs, fileDiff) return } - t.relevantFileDiffs = append(t.relevantFileDiffs, fileDiff) + if t.unwantedFiles == nil { + t.unwantedFiles = make(map[string]struct{}) + } + t.unwantedFiles[fileName] = struct{}{} } func (t *BumpKubevirtCI) Review() BotReviewResult { @@ -103,8 +95,8 @@ func (t *BumpKubevirtCI) Review() BotReviewResult { } } - for fileName, unwantedFiles := range t.unwantedFiles { - result.AddReviewFailure(fileName, unwantedFiles...) + for fileName := range t.unwantedFiles { + result.AddReviewFailure(fileName) } return result diff --git a/external-plugins/botreview/review/bump_kubevirtci_test.go b/external-plugins/botreview/review/bump_kubevirtci_test.go index 862be517a6..104f39a603 100644 --- a/external-plugins/botreview/review/bump_kubevirtci_test.go +++ b/external-plugins/botreview/review/bump_kubevirtci_test.go @@ -28,7 +28,25 @@ import ( ) func TestBumpKubevirtCI_Review(t1 *testing.T) { - diffFilePaths := []string{} + diffFilePaths := []string{ + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16", + } entries, err := os.ReadDir("testdata/kubevirtci-bump") if err != nil { t1.Errorf("failed to read files: %v", err) @@ -58,7 +76,7 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) { want BotReviewResult }{ { - name: "simple prow autobump", + name: "simple kubevirtci-bump", fields: fields{ relevantFileDiffs: []*diff.FileDiff{ diffFilePathsToDiffs["testdata/kubevirtci-bump/cluster-up-sha.txt"], @@ -74,7 +92,7 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) { want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, nil, ""), }, { - name: "mixed image bump", + name: "mixed kubevirtci-bump", fields: fields{ relevantFileDiffs: []*diff.FileDiff{ diffFilePathsToDiffs["testdata/kubevirtci-bump/cluster-up-sha.txt"], @@ -82,7 +100,50 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) { diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"], }, }, - want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, map[string][]*diff.Hunk{"github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml": diffFilePathsToDiffs["testdata/mixed_bump_prow_job.patch0"].Hunks}, ""), + want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, map[string][]*diff.Hunk{"github/ci/prow-deploy/files/jobs/kubevirt/kubevirt/kubevirt-presubmits.yaml": nil}, ""), + }, + { + name: "non kubevirtci bump", + fields: fields{ + relevantFileDiffs: []*diff.FileDiff{ + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16"], + }, + }, + want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, map[string][]*diff.Hunk{ + "pkg/virt-operator/resource/generate/components/daemonsets.go": nil, + "pkg/container-disk/container-disk_test.go": nil, + "pkg/virt-handler/container-disk/BUILD.bazel": nil, + "staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go": nil, + "staging/src/kubevirt.io/client-go/api/openapi_generated.go": nil, + "staging/src/kubevirt.io/api/core/v1/types.go": nil, + "api/openapi-spec/swagger.json": nil, + "pkg/virt-handler/container-disk/mount.go": nil, + "pkg/virt-controller/services/template.go": nil, + "staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go": nil, + "pkg/virt-operator/resource/generate/components/validations_generated.go": nil, + "pkg/virt-handler/vm.go": nil, + "pkg/virt-handler/container-disk/generated_mock_mount.go": nil, + "pkg/virt-handler/isolation/isolation.go": nil, + "pkg/virt-handler/container-disk/mount_test.go": nil, + "pkg/virt-handler/vm_test.go": nil, + "pkg/container-disk/container-disk.go": nil, + }, ""), }, } for _, tt := range tests { diff --git a/external-plugins/botreview/review/review.go b/external-plugins/botreview/review/review.go index 3495e089ee..add9e6368f 100644 --- a/external-plugins/botreview/review/review.go +++ b/external-plugins/botreview/review/review.go @@ -50,7 +50,7 @@ func GuessReviewTypes(fileDiffs []*diff.FileDiff) []KindOfChange { kindOfChange.AddIfRelevant(fileDiff) } } - result := []KindOfChange{} + var result []KindOfChange for _, t := range possibleReviewTypes { if t.IsRelevant() { result = append(result, t) diff --git a/external-plugins/botreview/review/review_test.go b/external-plugins/botreview/review/review_test.go index 2e2d43dfc5..08a02b20ed 100644 --- a/external-plugins/botreview/review/review_test.go +++ b/external-plugins/botreview/review/review_test.go @@ -32,6 +32,28 @@ func TestGuessReviewTypes(t *testing.T) { "testdata/move_prometheus_stack.patch0", "testdata/move_prometheus_stack.patch1", "testdata/cdi_arm_release.patch0", + "testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0", + "testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1", + "testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2", + "testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3", + "testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15", + "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16", } diffFilePathsToDiffs := map[string]*diff.FileDiff{} for _, diffFile := range diffFilePaths { @@ -87,23 +109,61 @@ func TestGuessReviewTypes(t *testing.T) { }, }, { - name: "non image bump should not yield a change", + name: "non image bump (move_prometheus_stack) should not yield a change", args: args{ fileDiffs: []*diff.FileDiff{ diffFilePathsToDiffs["testdata/move_prometheus_stack.patch0"], diffFilePathsToDiffs["testdata/move_prometheus_stack.patch1"], }, }, - want: []KindOfChange{}, + want: nil, }, { - name: "non image bump should not yield a change 2", + name: "non image bump (cdi_arm_release) should not yield a change 2", args: args{ fileDiffs: []*diff.FileDiff{ diffFilePathsToDiffs["testdata/cdi_arm_release.patch0"], }, }, - want: []KindOfChange{}, + want: nil, + }, + { + name: "non kubevirtci bump (eviction-strategy-doc) should not yield a change", + args: args{ + fileDiffs: []*diff.FileDiff{ + diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0"], + diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1"], + diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2"], + diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3"], + diffFilePathsToDiffs["testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4"], + }, + }, + want: nil, + }, + { + name: "non kubevirtci bump (fix-containerdisks-migration) should not yield a change", + args: args{ + fileDiffs: []*diff.FileDiff{ + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15"], + diffFilePathsToDiffs["testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16"], + }, + }, + want: nil, }, } for _, tt := range tests { diff --git a/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0 b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0 new file mode 100644 index 0000000000..53e7ac42d0 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch0 @@ -0,0 +1,13 @@ +a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json +index 44873ea7f..f69e61964 100644 +--- a/api/openapi-spec/swagger.json ++++ b/api/openapi-spec/swagger.json +@@ -20275,7 +20275,7 @@ + "$ref": "#/definitions/v1.DomainSpec" + }, + "evictionStrategy": { +- "description": "EvictionStrategy can be set to \"LiveMigrate\" if the VirtualMachineInstance should be migrated instead of shut-off in case of a node drain.", ++ "description": "EvictionStrategy describes the strategy to follow when a node drain occurs. The possible options are: - \"None\": No action will be taken, according to the specified 'RunStrategy' the VirtualMachine will be restarted or shutdown. - \"LiveMigrate\": the VirtualMachineInstance will be migrated instead of being shutdown. - \"LiveMigrateIfPossible\": the same as \"LiveMigrate\" but only if the VirtualMachine is Live-Migratable, otherwise it will behave as \"None\". - \"External\": the VirtualMachineInstance will be protected by a PDB and `vmi.Status.EvacuationNodeName` will be set on eviction. This is mainly useful for cluster-api-provider-kubevirt (capk) which needs a way for VMI's to be blocked from eviction, yet signal capk that eviction has been called on the VMI so the capk controller can handle tearing the VMI down. Details can be found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.", + "type": "string" + }, + "hostname": { diff --git a/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1 b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1 new file mode 100644 index 0000000000..fd4b33528c --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch1 @@ -0,0 +1,127 @@ +a/pkg/virt-operator/resource/generate/components/validations_generated.go b/pkg/virt-operator/resource/generate/components/validations_generated.go +index c3b49caa8..ff7a7d39e 100644 +--- a/pkg/virt-operator/resource/generate/components/validations_generated.go ++++ b/pkg/virt-operator/resource/generate/components/validations_generated.go +@@ -6498,9 +6498,20 @@ var CRDsValidation map[string]string = map[string]string{ + - devices + type: object + evictionStrategy: +- description: EvictionStrategy can be set to "LiveMigrate" if the +- VirtualMachineInstance should be migrated instead of shut-off +- in case of a node drain. ++ description: 'EvictionStrategy describes the strategy to follow ++ when a node drain occurs. The possible options are: - "None": ++ No action will be taken, according to the specified ''RunStrategy'' ++ the VirtualMachine will be restarted or shutdown. - "LiveMigrate": ++ the VirtualMachineInstance will be migrated instead of being shutdown. ++ - "LiveMigrateIfPossible": the same as "LiveMigrate" but only ++ if the VirtualMachine is Live-Migratable, otherwise it will behave ++ as "None". - "External": the VirtualMachineInstance will be protected ++ by a PDB and ''vmi.Status.EvacuationNodeName'' will be set on ++ eviction. This is mainly useful for cluster-api-provider-kubevirt ++ (capk) which needs a way for VMI''s to be blocked from eviction, ++ yet signal capk that eviction has been called on the VMI so the ++ capk controller can handle tearing the VMI down. Details can be ++ found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.' + type: string + hostname: + description: Specifies the hostname of the vmi If not specified, +@@ -10998,8 +11009,19 @@ var CRDsValidation map[string]string = map[string]string{ + - devices + type: object + evictionStrategy: +- description: EvictionStrategy can be set to "LiveMigrate" if the VirtualMachineInstance +- should be migrated instead of shut-off in case of a node drain. ++ description: 'EvictionStrategy describes the strategy to follow when a node ++ drain occurs. The possible options are: - "None": No action will be taken, ++ according to the specified ''RunStrategy'' the VirtualMachine will be ++ restarted or shutdown. - "LiveMigrate": the VirtualMachineInstance will ++ be migrated instead of being shutdown. - "LiveMigrateIfPossible": the ++ same as "LiveMigrate" but only if the VirtualMachine is Live-Migratable, ++ otherwise it will behave as "None". - "External": the VirtualMachineInstance ++ will be protected by a PDB and ''vmi.Status.EvacuationNodeName'' will ++ be set on eviction. This is mainly useful for cluster-api-provider-kubevirt ++ (capk) which needs a way for VMI''s to be blocked from eviction, yet signal ++ capk that eviction has been called on the VMI so the capk controller can ++ handle tearing the VMI down. Details can be found in the commit description ++ https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.' + type: string + hostname: + description: Specifies the hostname of the vmi If not specified, the hostname +@@ -15953,9 +15975,20 @@ var CRDsValidation map[string]string = map[string]string{ + - devices + type: object + evictionStrategy: +- description: EvictionStrategy can be set to "LiveMigrate" if the +- VirtualMachineInstance should be migrated instead of shut-off +- in case of a node drain. ++ description: 'EvictionStrategy describes the strategy to follow ++ when a node drain occurs. The possible options are: - "None": ++ No action will be taken, according to the specified ''RunStrategy'' ++ the VirtualMachine will be restarted or shutdown. - "LiveMigrate": ++ the VirtualMachineInstance will be migrated instead of being shutdown. ++ - "LiveMigrateIfPossible": the same as "LiveMigrate" but only ++ if the VirtualMachine is Live-Migratable, otherwise it will behave ++ as "None". - "External": the VirtualMachineInstance will be protected ++ by a PDB and ''vmi.Status.EvacuationNodeName'' will be set on ++ eviction. This is mainly useful for cluster-api-provider-kubevirt ++ (capk) which needs a way for VMI''s to be blocked from eviction, ++ yet signal capk that eviction has been called on the VMI so the ++ capk controller can handle tearing the VMI down. Details can be ++ found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.' + type: string + hostname: + description: Specifies the hostname of the vmi If not specified, +@@ -20371,9 +20404,22 @@ var CRDsValidation map[string]string = map[string]string{ + - devices + type: object + evictionStrategy: +- description: EvictionStrategy can be set to "LiveMigrate" +- if the VirtualMachineInstance should be migrated instead +- of shut-off in case of a node drain. ++ description: 'EvictionStrategy describes the strategy to ++ follow when a node drain occurs. The possible options ++ are: - "None": No action will be taken, according to the ++ specified ''RunStrategy'' the VirtualMachine will be restarted ++ or shutdown. - "LiveMigrate": the VirtualMachineInstance ++ will be migrated instead of being shutdown. - "LiveMigrateIfPossible": ++ the same as "LiveMigrate" but only if the VirtualMachine ++ is Live-Migratable, otherwise it will behave as "None". ++ - "External": the VirtualMachineInstance will be protected ++ by a PDB and ''vmi.Status.EvacuationNodeName'' will be ++ set on eviction. This is mainly useful for cluster-api-provider-kubevirt ++ (capk) which needs a way for VMI''s to be blocked from ++ eviction, yet signal capk that eviction has been called ++ on the VMI so the capk controller can handle tearing the ++ VMI down. Details can be found in the commit description ++ https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.' + type: string + hostname: + description: Specifies the hostname of the vmi If not specified, +@@ -25563,9 +25609,23 @@ var CRDsValidation map[string]string = map[string]string{ + - devices + type: object + evictionStrategy: +- description: EvictionStrategy can be set to "LiveMigrate" +- if the VirtualMachineInstance should be migrated instead +- of shut-off in case of a node drain. ++ description: 'EvictionStrategy describes the strategy ++ to follow when a node drain occurs. The possible options ++ are: - "None": No action will be taken, according ++ to the specified ''RunStrategy'' the VirtualMachine ++ will be restarted or shutdown. - "LiveMigrate": the ++ VirtualMachineInstance will be migrated instead of ++ being shutdown. - "LiveMigrateIfPossible": the same ++ as "LiveMigrate" but only if the VirtualMachine is ++ Live-Migratable, otherwise it will behave as "None". ++ - "External": the VirtualMachineInstance will be protected ++ by a PDB and ''vmi.Status.EvacuationNodeName'' will ++ be set on eviction. This is mainly useful for cluster-api-provider-kubevirt ++ (capk) which needs a way for VMI''s to be blocked ++ from eviction, yet signal capk that eviction has been ++ called on the VMI so the capk controller can handle ++ tearing the VMI down. Details can be found in the ++ commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.' + type: string + hostname: + description: Specifies the hostname of the vmi If not diff --git a/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2 b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2 new file mode 100644 index 0000000000..957a820673 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch2 @@ -0,0 +1,20 @@ +a/staging/src/kubevirt.io/api/core/v1/types.go b/staging/src/kubevirt.io/api/core/v1/types.go +index 130b1520e..81f841182 100644 +--- a/staging/src/kubevirt.io/api/core/v1/types.go ++++ b/staging/src/kubevirt.io/api/core/v1/types.go +@@ -109,9 +109,12 @@ type VirtualMachineInstanceSpec struct { + // +listMapKey=topologyKey + // +listMapKey=whenUnsatisfiable + TopologySpreadConstraints []k8sv1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty" patchStrategy:"merge" patchMergeKey:"topologyKey"` +- // EvictionStrategy can be set to "LiveMigrate" if the VirtualMachineInstance should be +- // migrated instead of shut-off in case of a node drain. +- // ++ // EvictionStrategy describes the strategy to follow when a node drain occurs. ++ // The possible options are: ++ // - "None": No action will be taken, according to the specified 'RunStrategy' the VirtualMachine will be restarted or shutdown. ++ // - "LiveMigrate": the VirtualMachineInstance will be migrated instead of being shutdown. ++ // - "LiveMigrateIfPossible": the same as "LiveMigrate" but only if the VirtualMachine is Live-Migratable, otherwise it will behave as "None". ++ // - "External": the VirtualMachineInstance will be protected by a PDB and `vmi.Status.EvacuationNodeName` will be set on eviction. This is mainly useful for cluster-api-provider-kubevirt (capk) which needs a way for VMI's to be blocked from eviction, yet signal capk that eviction has been called on the VMI so the capk controller can handle tearing the VMI down. Details can be found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa. + // +optional + EvictionStrategy *EvictionStrategy `json:"evictionStrategy,omitempty"` + // StartStrategy can be set to "Paused" if Virtual Machine should be started in paused state. diff --git a/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3 b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3 new file mode 100644 index 0000000000..7752ece689 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch3 @@ -0,0 +1,13 @@ +a/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go b/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go +index 9a106468b..dfbf8fdc0 100644 +--- a/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go ++++ b/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go +@@ -26,7 +26,7 @@ func (VirtualMachineInstanceSpec) SwaggerDoc() map[string]string { + "schedulerName": "If specified, the VMI will be dispatched by specified scheduler.\nIf not specified, the VMI will be dispatched by default scheduler.\n+optional", + "tolerations": "If toleration is specified, obey all the toleration rules.", + "topologySpreadConstraints": "TopologySpreadConstraints describes how a group of VMIs will be spread across a given topology\ndomains. K8s scheduler will schedule VMI pods in a way which abides by the constraints.\n+optional\n+patchMergeKey=topologyKey\n+patchStrategy=merge\n+listType=map\n+listMapKey=topologyKey\n+listMapKey=whenUnsatisfiable", +- "evictionStrategy": "EvictionStrategy can be set to \"LiveMigrate\" if the VirtualMachineInstance should be\nmigrated instead of shut-off in case of a node drain.\n\n+optional", ++ "evictionStrategy": "EvictionStrategy describes the strategy to follow when a node drain occurs.\nThe possible options are:\n- \"None\": No action will be taken, according to the specified 'RunStrategy' the VirtualMachine will be restarted or shutdown.\n- \"LiveMigrate\": the VirtualMachineInstance will be migrated instead of being shutdown.\n- \"LiveMigrateIfPossible\": the same as \"LiveMigrate\" but only if the VirtualMachine is Live-Migratable, otherwise it will behave as \"None\".\n- \"External\": the VirtualMachineInstance will be protected by a PDB and `vmi.Status.EvacuationNodeName` will be set on eviction. This is mainly useful for cluster-api-provider-kubevirt (capk) which needs a way for VMI's to be blocked from eviction, yet signal capk that eviction has been called on the VMI so the capk controller can handle tearing the VMI down. Details can be found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.\n+optional", + "startStrategy": "StartStrategy can be set to \"Paused\" if Virtual Machine should be started in paused state.\n\n+optional", + "terminationGracePeriodSeconds": "Grace period observed after signalling a VirtualMachineInstance to stop after which the VirtualMachineInstance is force terminated.", + "volumes": "List of volumes that can be mounted by disks belonging to the vmi.", diff --git a/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4 b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4 new file mode 100644 index 0000000000..fa215a62c2 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/eviction-strategy-doc/eviction-strategy-doc.patch4 @@ -0,0 +1,13 @@ +a/staging/src/kubevirt.io/client-go/api/openapi_generated.go b/staging/src/kubevirt.io/client-go/api/openapi_generated.go +index ca73ca38a..cb480752f 100644 +--- a/staging/src/kubevirt.io/client-go/api/openapi_generated.go ++++ b/staging/src/kubevirt.io/client-go/api/openapi_generated.go +@@ -24018,7 +24018,7 @@ func schema_kubevirtio_api_core_v1_VirtualMachineInstanceSpec(ref common.Referen + }, + "evictionStrategy": { + SchemaProps: spec.SchemaProps{ +- Description: "EvictionStrategy can be set to \"LiveMigrate\" if the VirtualMachineInstance should be migrated instead of shut-off in case of a node drain.", ++ Description: "EvictionStrategy describes the strategy to follow when a node drain occurs. The possible options are: - \"None\": No action will be taken, according to the specified 'RunStrategy' the VirtualMachine will be restarted or shutdown. - \"LiveMigrate\": the VirtualMachineInstance will be migrated instead of being shutdown. - \"LiveMigrateIfPossible\": the same as \"LiveMigrate\" but only if the VirtualMachine is Live-Migratable, otherwise it will behave as \"None\". - \"External\": the VirtualMachineInstance will be protected by a PDB and `vmi.Status.EvacuationNodeName` will be set on eviction. This is mainly useful for cluster-api-provider-kubevirt (capk) which needs a way for VMI's to be blocked from eviction, yet signal capk that eviction has been called on the VMI so the capk controller can handle tearing the VMI down. Details can be found in the commit description https://github.com/kubevirt/kubevirt/commit/c1d77face705c8b126696bac9a3ee3825f27f1fa.", + Type: []string{"string"}, + Format: "", + }, diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00 new file mode 100644 index 0000000000..c8726292b0 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch00 @@ -0,0 +1,94 @@ +a/api/openapi-spec/swagger.json b/api/openapi-spec/swagger.json +index 6332d8d63..478400757 100644 +--- a/api/openapi-spec/swagger.json ++++ b/api/openapi-spec/swagger.json +@@ -16152,6 +16152,17 @@ + } + } + }, ++ "v1.ContainerDiskInfo": { ++ "description": "ContainerDiskInfo shows info about the containerdisk", ++ "type": "object", ++ "properties": { ++ "checksum": { ++ "description": "Checksum is the checksum of the rootdisk or kernel artifacts inside the containerdisk", ++ "type": "integer", ++ "format": "int64" ++ } ++ } ++ }, + "v1.ContainerDiskSource": { + "description": "Represents a docker image with an embedded disk.", + "type": "object", +@@ -17214,6 +17225,17 @@ + } + } + }, ++ "v1.InitrdInfo": { ++ "description": "InitrdInfo show info about the initrd file", ++ "type": "object", ++ "properties": { ++ "checksum": { ++ "description": "Checksum is the checksum of the initrd file", ++ "type": "integer", ++ "format": "int64" ++ } ++ } ++ }, + "v1.Input": { + "type": "object", + "required": [ +@@ -17445,6 +17467,31 @@ + } + } + }, ++ "v1.KernelBootStatus": { ++ "description": "KernelBootStatus contains info about the kernelBootContainer", ++ "type": "object", ++ "properties": { ++ "initrdInfo": { ++ "description": "InitrdInfo show info about the initrd file", ++ "$ref": "#/definitions/v1.InitrdInfo" ++ }, ++ "kernelInfo": { ++ "description": "KernelInfo show info about the kernel image", ++ "$ref": "#/definitions/v1.KernelInfo" ++ } ++ } ++ }, ++ "v1.KernelInfo": { ++ "description": "KernelInfo show info about the kernel image", ++ "type": "object", ++ "properties": { ++ "checksum": { ++ "description": "Checksum is the checksum of the kernel image", ++ "type": "integer", ++ "format": "int64" ++ } ++ } ++ }, + "v1.KubeVirt": { + "description": "KubeVirt represents the object deploying all KubeVirt resources", + "type": "object", +@@ -20419,6 +20466,10 @@ + "$ref": "#/definitions/v1.VirtualMachineInstanceNetworkInterface" + } + }, ++ "kernelBootStatus": { ++ "description": "KernelBootStatus contains info about the kernelBootContainer", ++ "$ref": "#/definitions/v1.KernelBootStatus" ++ }, + "launcherContainerImageVersion": { + "description": "LauncherContainerImageVersion indicates what container image is currently active for the vmi.", + "type": "string" +@@ -20875,6 +20926,10 @@ + "target" + ], + "properties": { ++ "containerDiskVolume": { ++ "description": "ContainerDiskVolume shows info about the containerdisk, if the volume is a containerdisk", ++ "$ref": "#/definitions/v1.ContainerDiskInfo" ++ }, + "hotplugVolume": { + "description": "If the volume is hotplug, this will contain the hotplug status.", + "$ref": "#/definitions/v1.HotplugVolumeStatus" diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01 new file mode 100644 index 0000000000..85b2a9da52 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch01 @@ -0,0 +1,49 @@ +a/pkg/container-disk/container-disk.go b/pkg/container-disk/container-disk.go +index f16e86c51..3251d0478 100644 +--- a/pkg/container-disk/container-disk.go ++++ b/pkg/container-disk/container-disk.go +@@ -399,9 +399,9 @@ func getContainerDiskSocketBasePath(baseDir, podUID string) string { + } + + // ExtractImageIDsFromSourcePod takes the VMI and its source pod to determine the exact image used by containerdisks and boot container images, +-// which is recorded in the status section of a started pod. ++// which is recorded in the status section of a started pod; if the status section does not contain this info the tag is used. + // It returns a map where the key is the vlume name and the value is the imageID +-func ExtractImageIDsFromSourcePod(vmi *v1.VirtualMachineInstance, sourcePod *kubev1.Pod) (imageIDs map[string]string, err error) { ++func ExtractImageIDsFromSourcePod(vmi *v1.VirtualMachineInstance, sourcePod *kubev1.Pod) (imageIDs map[string]string) { + imageIDs = map[string]string{} + for _, volume := range vmi.Spec.Volumes { + if volume.ContainerDisk == nil { +@@ -423,16 +423,12 @@ func ExtractImageIDsFromSourcePod(vmi *v1.VirtualMachineInstance, sourcePod *kub + if !exists { + continue + } +- imageID, err := toImageWithDigest(image, status.ImageID) +- if err != nil { +- return nil, err +- } +- imageIDs[key] = imageID ++ imageIDs[key] = toPullableImageReference(image, status.ImageID) + } + return + } + +-func toImageWithDigest(image string, imageID string) (string, error) { ++func toPullableImageReference(image string, imageID string) string { + baseImage := image + if strings.LastIndex(image, "@sha256:") != -1 { + baseImage = strings.Split(image, "@sha256:")[0] +@@ -442,9 +438,11 @@ func toImageWithDigest(image string, imageID string) (string, error) { + + digestMatches := digestRegex.FindStringSubmatch(imageID) + if len(digestMatches) < 2 { +- return "", fmt.Errorf("failed to identify image digest for container %q with id %q", image, imageID) ++ // failed to identify image digest for container, will use the image tag ++ // as virt-handler will anyway check the checksum of the root disk image ++ return image + } +- return fmt.Sprintf("%s@sha256:%s", baseImage, digestMatches[1]), nil ++ return fmt.Sprintf("%s@sha256:%s", baseImage, digestMatches[1]) + } + + func isImageVolume(containerName string) bool { diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02 new file mode 100644 index 0000000000..c755783980 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch02 @@ -0,0 +1,77 @@ +a/pkg/container-disk/container-disk_test.go b/pkg/container-disk/container-disk_test.go +index ee18803ad..5baf92f8b 100644 +--- a/pkg/container-disk/container-disk_test.go ++++ b/pkg/container-disk/container-disk_test.go +@@ -357,8 +357,7 @@ var _ = Describe("ContainerDisk", func() { + + pod := createMigrationSourcePod(vmi) + +- imageIDs, err := ExtractImageIDsFromSourcePod(vmi, pod) +- Expect(err).ToNot(HaveOccurred()) ++ imageIDs := ExtractImageIDsFromSourcePod(vmi, pod) + Expect(imageIDs).To(HaveKeyWithValue("disk1", "someimage@sha256:0")) + Expect(imageIDs).To(HaveKeyWithValue("disk2", "someimage@sha256:1")) + Expect(imageIDs).To(HaveLen(2)) +@@ -379,8 +378,7 @@ var _ = Describe("ContainerDisk", func() { + + pod := createMigrationSourcePod(vmi) + +- imageIDs, err := ExtractImageIDsFromSourcePod(vmi, pod) +- Expect(err).ToNot(HaveOccurred()) ++ imageIDs := ExtractImageIDsFromSourcePod(vmi, pod) + Expect(imageIDs).To(HaveKeyWithValue("disk1", "someimage@sha256:0")) + Expect(imageIDs).To(HaveKeyWithValue("kernel-boot-volume", "someimage@sha256:bootcontainer")) + Expect(imageIDs).To(HaveLen(2)) +@@ -392,26 +390,22 @@ var _ = Describe("ContainerDisk", func() { + Expect(newContainers[1].Image).To(Equal("someimage@sha256:bootcontainer")) + }) + +- It("should fail if it can't detect a reproducible imageID", func() { ++ It("should return the source image tag if it can't detect a reproducible imageID", func() { + vmi := api.NewMinimalVMI("myvmi") + appendContainerDisk(vmi, "disk1") + pod := createMigrationSourcePod(vmi) + pod.Status.ContainerStatuses[0].ImageID = "rubbish" +- _, err := ExtractImageIDsFromSourcePod(vmi, pod) +- Expect(err).To(HaveOccurred()) +- Expect(err.Error()).To(Equal(`failed to identify image digest for container "someimage:v1.2.3.4" with id "rubbish"`)) ++ imageIDs := ExtractImageIDsFromSourcePod(vmi, pod) ++ Expect(imageIDs["disk1"]).To(Equal(vmi.Spec.Volumes[0].ContainerDisk.Image)) + }) + + DescribeTable("It should detect the image ID from", func(imageID string) { + expected := "myregistry.io/myimage@sha256:4gjffGJlg4" +- res, err := toImageWithDigest("myregistry.io/myimage", imageID) +- Expect(err).ToNot(HaveOccurred()) ++ res := toPullableImageReference("myregistry.io/myimage", imageID) + Expect(res).To(Equal(expected)) +- res, err = toImageWithDigest("myregistry.io/myimage:1234", imageID) +- Expect(err).ToNot(HaveOccurred()) ++ res = toPullableImageReference("myregistry.io/myimage:1234", imageID) + Expect(res).To(Equal(expected)) +- res, err = toImageWithDigest("myregistry.io/myimage:latest", imageID) +- Expect(err).ToNot(HaveOccurred()) ++ res = toPullableImageReference("myregistry.io/myimage:latest", imageID) + Expect(res).To(Equal(expected)) + }, + Entry("docker", "docker://sha256:4gjffGJlg4"), +@@ -420,8 +414,7 @@ var _ = Describe("ContainerDisk", func() { + ) + + DescribeTable("It should detect the base image from", func(given, expected string) { +- res, err := toImageWithDigest(given, "docker://sha256:4gjffGJlg4") +- Expect(err).ToNot(HaveOccurred()) ++ res := toPullableImageReference(given, "docker://sha256:4gjffGJlg4") + Expect(strings.Split(res, "@sha256:")[0]).To(Equal(expected)) + }, + Entry("image with registry and no tags or shasum", "myregistry.io/myimage", "myregistry.io/myimage"), +@@ -443,8 +436,7 @@ var _ = Describe("ContainerDisk", func() { + appendContainerDisk(vmi, "disk1") + + pod := createMigrationSourcePod(vmi) +- imageIDs, err := ExtractImageIDsFromSourcePod(vmi, pod) +- Expect(err).ToNot(HaveOccurred()) ++ imageIDs := ExtractImageIDsFromSourcePod(vmi, pod) + + newContainers := GenerateContainers(vmi, clusterConfig, imageIDs, "a-name", "something") + testFunc(&newContainers[0]) diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03 new file mode 100644 index 0000000000..7ac9193e62 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch03 @@ -0,0 +1,18 @@ +a/pkg/virt-controller/services/template.go b/pkg/virt-controller/services/template.go +index e7802f6d9..7a2fe645c 100644 +--- a/pkg/virt-controller/services/template.go ++++ b/pkg/virt-controller/services/template.go +@@ -260,11 +260,8 @@ func (t *templateService) RenderLaunchManifestNoVm(vmi *v1.VirtualMachineInstanc + } + + func (t *templateService) RenderMigrationManifest(vmi *v1.VirtualMachineInstance, pod *k8sv1.Pod) (*k8sv1.Pod, error) { +- reproducibleImageIDs, err := containerdisk.ExtractImageIDsFromSourcePod(vmi, pod) +- if err != nil { +- return nil, fmt.Errorf("can not proceed with the migration when no reproducible image digest can be detected: %v", err) +- } +- podManifest, err := t.renderLaunchManifest(vmi, reproducibleImageIDs, false) ++ imageIDs := containerdisk.ExtractImageIDsFromSourcePod(vmi, pod) ++ podManifest, err := t.renderLaunchManifest(vmi, imageIDs, false) + if err != nil { + return nil, err + } diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04 new file mode 100644 index 0000000000..09f698bbb1 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch04 @@ -0,0 +1,22 @@ +a/pkg/virt-handler/container-disk/BUILD.bazel b/pkg/virt-handler/container-disk/BUILD.bazel +index 476389a08..9ff4f7c4e 100644 +--- a/pkg/virt-handler/container-disk/BUILD.bazel ++++ b/pkg/virt-handler/container-disk/BUILD.bazel +@@ -35,12 +35,17 @@ go_test( + deps = [ + "//pkg/container-disk:go_default_library", + "//pkg/ephemeral-disk-utils:go_default_library", ++ "//pkg/safepath:go_default_library", + "//pkg/testutils:go_default_library", ++ "//pkg/virt-handler/isolation:go_default_library", + "//staging/src/kubevirt.io/api/core/v1:go_default_library", + "//staging/src/kubevirt.io/client-go/api:go_default_library", + "//staging/src/kubevirt.io/client-go/testutils:go_default_library", ++ "//vendor/github.com/golang/mock/gomock:go_default_library", ++ "//vendor/github.com/moby/sys/mountinfo:go_default_library", + "//vendor/github.com/onsi/ginkgo/v2:go_default_library", + "//vendor/github.com/onsi/gomega:go_default_library", ++ "//vendor/github.com/onsi/gomega/types:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", + ], + ) diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05 new file mode 100644 index 0000000000..4cf60c299c --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch05 @@ -0,0 +1,19 @@ +a/pkg/virt-handler/container-disk/generated_mock_mount.go b/pkg/virt-handler/container-disk/generated_mock_mount.go +index 1af16221c..906728415 100644 +--- a/pkg/virt-handler/container-disk/generated_mock_mount.go ++++ b/pkg/virt-handler/container-disk/generated_mock_mount.go +@@ -64,3 +64,14 @@ func (_m *MockMounter) Unmount(vmi *v1.VirtualMachineInstance) error { + func (_mr *_MockMounterRecorder) Unmount(arg0 interface{}) *gomock.Call { + return _mr.mock.ctrl.RecordCall(_mr.mock, "Unmount", arg0) + } ++ ++func (_m *MockMounter) ComputeChecksums(vmi *v1.VirtualMachineInstance) (*DiskChecksums, error) { ++ ret := _m.ctrl.Call(_m, "ComputeChecksums", vmi) ++ ret0, _ := ret[0].(*DiskChecksums) ++ ret1, _ := ret[1].(error) ++ return ret0, ret1 ++} ++ ++func (_mr *_MockMounterRecorder) ComputeChecksums(arg0 interface{}) *gomock.Call { ++ return _mr.mock.ctrl.RecordCall(_mr.mock, "ComputeChecksums", arg0) ++} diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06 new file mode 100644 index 0000000000..38cd62be4a --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch06 @@ -0,0 +1,371 @@ +a/pkg/virt-handler/container-disk/mount.go b/pkg/virt-handler/container-disk/mount.go +index 6e1d350fe..9d9cb464b 100644 +--- a/pkg/virt-handler/container-disk/mount.go ++++ b/pkg/virt-handler/container-disk/mount.go +@@ -4,6 +4,8 @@ import ( + "encoding/json" + "errors" + "fmt" ++ "hash/crc32" ++ "io" + "os" + "path/filepath" + "strings" +@@ -34,6 +36,12 @@ const ( + failedUnmountFmt = "failed to unmount containerDisk %v: %v : %v" + ) + ++var ( ++ ErrChecksumMissing = errors.New("missing checksum") ++ ErrChecksumMismatch = errors.New("checksum mismatch") ++ ErrDiskContainerGone = errors.New("disk container is gone") ++) ++ + //go:generate mockgen -source $GOFILE -package=$GOPACKAGE -destination=generated_mock_$GOFILE + + type mounter struct { +@@ -45,12 +53,14 @@ type mounter struct { + socketPathGetter containerdisk.SocketPathGetter + kernelBootSocketPathGetter containerdisk.KernelBootSocketPathGetter + clusterConfig *virtconfig.ClusterConfig ++ nodeIsolationResult isolation.IsolationResult + } + + type Mounter interface { + ContainerDisksReady(vmi *v1.VirtualMachineInstance, notInitializedSince time.Time) (bool, error) + MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*containerdisk.DiskInfo, error) + Unmount(vmi *v1.VirtualMachineInstance) error ++ ComputeChecksums(vmi *v1.VirtualMachineInstance) (*DiskChecksums, error) + } + + type vmiMountTargetEntry struct { +@@ -63,6 +73,21 @@ type vmiMountTargetRecord struct { + UsesSafePaths bool `json:"usesSafePaths"` + } + ++type kernelArtifacts struct { ++ kernel *safepath.Path ++ initrd *safepath.Path ++} ++ ++type DiskChecksums struct { ++ KernelBootChecksum KernelBootChecksum ++ ContainerDiskChecksums map[string]uint32 ++} ++ ++type KernelBootChecksum struct { ++ Initrd *uint32 ++ Kernel *uint32 ++} ++ + func NewMounter(isoDetector isolation.PodIsolationDetector, mountStateDir string, clusterConfig *virtconfig.ClusterConfig) Mounter { + return &mounter{ + mountRecords: make(map[types.UID]*vmiMountTargetRecord), +@@ -72,6 +97,7 @@ func NewMounter(isoDetector isolation.PodIsolationDetector, mountStateDir string + socketPathGetter: containerdisk.NewSocketPathGetter(""), + kernelBootSocketPathGetter: containerdisk.NewKernelBootSocketPathGetter(""), + clusterConfig: clusterConfig, ++ nodeIsolationResult: isolation.NodeIsolationResult(), + } + } + +@@ -281,25 +307,11 @@ func (m *mounter) MountAndVerify(vmi *v1.VirtualMachineInstance) (map[string]*co + return nil, err + } + +- nodeRes := isolation.NodeIsolationResult() +- + if isMounted, err := isolation.IsMounted(targetFile); err != nil { + return nil, fmt.Errorf("failed to determine if %s is already mounted: %v", targetFile, err) + } else if !isMounted { +- sock, err := m.socketPathGetter(vmi, i) +- if err != nil { +- return nil, err +- } + +- res, err := m.podIsolationDetector.DetectForSocket(vmi, sock) +- if err != nil { +- return nil, fmt.Errorf("failed to detect socket for containerDisk %v: %v", volume.Name, err) +- } +- mountPoint, err := isolation.ParentPathForRootMount(nodeRes, res) +- if err != nil { +- return nil, fmt.Errorf("failed to detect root mount point of containerDisk %v on the node: %v", volume.Name, err) +- } +- sourceFile, err := containerdisk.GetImage(mountPoint, volume.ContainerDisk.Path) ++ sourceFile, err := m.getContainerDiskPath(vmi, &volume, i) + if err != nil { + return nil, fmt.Errorf("failed to find a sourceFile in containerDisk %v: %v", volume.Name, err) + } +@@ -446,8 +458,6 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo + return err + } + +- nodeRes := isolation.NodeIsolationResult() +- + var targetInitrdPath *safepath.Path + var targetKernelPath *safepath.Path + +@@ -480,8 +490,15 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo + return false, err + } + +- mounted, err := nodeRes.AreMounted(artifactFiles...) +- return mounted, err ++ for _, mountPoint := range artifactFiles { ++ if mountPoint != nil { ++ isMounted, err := isolation.IsMounted(mountPoint) ++ if !isMounted || err != nil { ++ return isMounted, err ++ } ++ } ++ } ++ return true, nil + } + + if isMounted, err := areKernelArtifactsMounted(targetDir, targetInitrdPath, targetKernelPath); err != nil { +@@ -489,39 +506,22 @@ func (m *mounter) mountKernelArtifacts(vmi *v1.VirtualMachineInstance, verify bo + } else if !isMounted { + log.Log.Object(vmi).Infof("kernel artifacts are not mounted - mounting...") + +- res, err := m.podIsolationDetector.DetectForSocket(vmi, socketFilePath) +- if err != nil { +- return fmt.Errorf("failed to detect socket for containerDisk %v: %v", kernelBootName, err) +- } +- mountRootPath, err := isolation.ParentPathForRootMount(nodeRes, res) ++ kernelArtifacts, err := m.getKernelArtifactPaths(vmi) + if err != nil { +- return fmt.Errorf("failed to detect root mount point of %v on the node: %v", kernelBootName, err) ++ return err + } + +- mount := func(artifactPath string, targetPath *safepath.Path) error { +- +- sourcePath, err := containerdisk.GetImage(mountRootPath, artifactPath) +- if err != nil { +- return err +- } +- +- out, err := virt_chroot.MountChroot(sourcePath, targetPath, true).CombinedOutput() ++ if kernelArtifacts.kernel != nil { ++ out, err := virt_chroot.MountChroot(kernelArtifacts.kernel, targetKernelPath, true).CombinedOutput() + if err != nil { + return fmt.Errorf("failed to bindmount %v: %v : %v", kernelBootName, string(out), err) + } +- +- return nil + } + +- if kb.InitrdPath != "" { +- if err = mount(kb.InitrdPath, targetInitrdPath); err != nil { +- return err +- } +- } +- +- if kb.KernelPath != "" { +- if err = mount(kb.KernelPath, targetKernelPath); err != nil { +- return err ++ if kernelArtifacts.initrd != nil { ++ out, err := virt_chroot.MountChroot(kernelArtifacts.initrd, targetInitrdPath, true).CombinedOutput() ++ if err != nil { ++ return fmt.Errorf("failed to bindmount %v: %v : %v", kernelBootName, string(out), err) + } + } + +@@ -609,3 +609,197 @@ func (m *mounter) unmountKernelArtifacts(vmi *v1.VirtualMachineInstance) error { + + return fmt.Errorf("kernel artifacts record wasn't found") + } ++ ++func (m *mounter) getContainerDiskPath(vmi *v1.VirtualMachineInstance, volume *v1.Volume, volumeIndex int) (*safepath.Path, error) { ++ sock, err := m.socketPathGetter(vmi, volumeIndex) ++ if err != nil { ++ return nil, ErrDiskContainerGone ++ } ++ ++ res, err := m.podIsolationDetector.DetectForSocket(vmi, sock) ++ if err != nil { ++ return nil, fmt.Errorf("failed to detect socket for containerDisk %v: %v", volume.Name, err) ++ } ++ ++ mountPoint, err := isolation.ParentPathForRootMount(m.nodeIsolationResult, res) ++ if err != nil { ++ return nil, fmt.Errorf("failed to detect root mount point of containerDisk %v on the node: %v", volume.Name, err) ++ } ++ ++ return containerdisk.GetImage(mountPoint, volume.ContainerDisk.Path) ++} ++ ++func (m *mounter) getKernelArtifactPaths(vmi *v1.VirtualMachineInstance) (*kernelArtifacts, error) { ++ sock, err := m.kernelBootSocketPathGetter(vmi) ++ if err != nil { ++ return nil, ErrDiskContainerGone ++ } ++ ++ res, err := m.podIsolationDetector.DetectForSocket(vmi, sock) ++ if err != nil { ++ return nil, fmt.Errorf("failed to detect socket for kernelboot container: %v", err) ++ } ++ ++ mountPoint, err := isolation.ParentPathForRootMount(m.nodeIsolationResult, res) ++ if err != nil { ++ return nil, fmt.Errorf("failed to detect root mount point of kernel/initrd container on the node: %v", err) ++ } ++ ++ kernelContainer := vmi.Spec.Domain.Firmware.KernelBoot.Container ++ kernelArtifacts := &kernelArtifacts{} ++ ++ if kernelContainer.KernelPath != "" { ++ kernelPath, err := containerdisk.GetImage(mountPoint, kernelContainer.KernelPath) ++ if err != nil { ++ return nil, err ++ } ++ kernelArtifacts.kernel = kernelPath ++ } ++ if kernelContainer.InitrdPath != "" { ++ initrdPath, err := containerdisk.GetImage(mountPoint, kernelContainer.InitrdPath) ++ if err != nil { ++ return nil, err ++ } ++ kernelArtifacts.initrd = initrdPath ++ } ++ ++ return kernelArtifacts, nil ++} ++ ++func getDigest(imageFile *safepath.Path) (uint32, error) { ++ digest := crc32.NewIEEE() ++ ++ err := imageFile.ExecuteNoFollow(func(path string) (err error) { ++ f, err := os.Open(path) ++ if err != nil { ++ return err ++ } ++ defer f.Close() ++ ++ // 32 MiB chunks ++ chunk := make([]byte, 1024*1024*32) ++ ++ _, err = io.CopyBuffer(digest, f, chunk) ++ return err ++ }) ++ ++ return digest.Sum32(), err ++} ++ ++func (m *mounter) ComputeChecksums(vmi *v1.VirtualMachineInstance) (*DiskChecksums, error) { ++ ++ diskChecksums := &DiskChecksums{ ++ ContainerDiskChecksums: map[string]uint32{}, ++ } ++ ++ // compute for containerdisks ++ for i, volume := range vmi.Spec.Volumes { ++ if volume.VolumeSource.ContainerDisk == nil { ++ continue ++ } ++ ++ path, err := m.getContainerDiskPath(vmi, &volume, i) ++ if err != nil { ++ return nil, err ++ } ++ ++ checksum, err := getDigest(path) ++ if err != nil { ++ return nil, err ++ } ++ ++ diskChecksums.ContainerDiskChecksums[volume.Name] = checksum ++ } ++ ++ // kernel and initrd ++ if util.HasKernelBootContainerImage(vmi) { ++ kernelArtifacts, err := m.getKernelArtifactPaths(vmi) ++ if err != nil { ++ return nil, err ++ } ++ ++ if kernelArtifacts.kernel != nil { ++ checksum, err := getDigest(kernelArtifacts.kernel) ++ if err != nil { ++ return nil, err ++ } ++ ++ diskChecksums.KernelBootChecksum.Kernel = &checksum ++ } ++ ++ if kernelArtifacts.initrd != nil { ++ checksum, err := getDigest(kernelArtifacts.initrd) ++ if err != nil { ++ return nil, err ++ } ++ ++ diskChecksums.KernelBootChecksum.Initrd = &checksum ++ } ++ } ++ ++ return diskChecksums, nil ++} ++ ++func compareChecksums(expectedChecksum, computedChecksum uint32) error { ++ if expectedChecksum == 0 { ++ return ErrChecksumMissing ++ } ++ if expectedChecksum != computedChecksum { ++ return ErrChecksumMismatch ++ } ++ // checksum ok ++ return nil ++} ++ ++func VerifyChecksums(mounter Mounter, vmi *v1.VirtualMachineInstance) error { ++ diskChecksums, err := mounter.ComputeChecksums(vmi) ++ if err != nil { ++ return fmt.Errorf("failed to compute checksums: %s", err) ++ } ++ ++ // verify containerdisks ++ for _, volumeStatus := range vmi.Status.VolumeStatus { ++ if volumeStatus.ContainerDiskVolume == nil { ++ continue ++ } ++ ++ expectedChecksum := volumeStatus.ContainerDiskVolume.Checksum ++ computedChecksum := diskChecksums.ContainerDiskChecksums[volumeStatus.Name] ++ if err := compareChecksums(expectedChecksum, computedChecksum); err != nil { ++ return fmt.Errorf("checksum error for volume %s: %w", volumeStatus.Name, err) ++ } ++ } ++ ++ // verify kernel and initrd ++ if util.HasKernelBootContainerImage(vmi) { ++ if vmi.Status.KernelBootStatus == nil { ++ return ErrChecksumMissing ++ } ++ ++ if diskChecksums.KernelBootChecksum.Kernel != nil { ++ if vmi.Status.KernelBootStatus.KernelInfo == nil { ++ return fmt.Errorf("checksum missing for kernel image: %w", ErrChecksumMissing) ++ } ++ ++ expectedChecksum := vmi.Status.KernelBootStatus.KernelInfo.Checksum ++ computedChecksum := *diskChecksums.KernelBootChecksum.Kernel ++ if err := compareChecksums(expectedChecksum, computedChecksum); err != nil { ++ return fmt.Errorf("checksum error for kernel image: %w", err) ++ } ++ } ++ ++ if diskChecksums.KernelBootChecksum.Initrd != nil { ++ if vmi.Status.KernelBootStatus.InitrdInfo == nil { ++ return fmt.Errorf("checksum missing for initrd image: %w", ErrChecksumMissing) ++ } ++ ++ expectedChecksum := vmi.Status.KernelBootStatus.InitrdInfo.Checksum ++ computedChecksum := *diskChecksums.KernelBootChecksum.Initrd ++ if err := compareChecksums(expectedChecksum, computedChecksum); err != nil { ++ return fmt.Errorf("checksum error for initrd image: %w", err) ++ } ++ } ++ } ++ ++ return nil ++} diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07 new file mode 100644 index 0000000000..df4b8dc4df --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch07 @@ -0,0 +1,219 @@ +a/pkg/virt-handler/container-disk/mount_test.go b/pkg/virt-handler/container-disk/mount_test.go +index b0f3b496f..bb34a07cd 100644 +--- a/pkg/virt-handler/container-disk/mount_test.go ++++ b/pkg/virt-handler/container-disk/mount_test.go +@@ -21,16 +21,22 @@ package container_disk + + import ( + "fmt" ++ "hash/crc32" + "os" + "path/filepath" + "time" + + "kubevirt.io/client-go/api" + ++ "kubevirt.io/kubevirt/pkg/safepath" + "kubevirt.io/kubevirt/pkg/testutils" ++ "kubevirt.io/kubevirt/pkg/virt-handler/isolation" + ++ gomock "github.com/golang/mock/gomock" ++ mount "github.com/moby/sys/mountinfo" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" ++ gomega_types "github.com/onsi/gomega/types" + + containerdisk "kubevirt.io/kubevirt/pkg/container-disk" + +@@ -174,4 +180,191 @@ var _ = Describe("ContainerDisk", func() { + Expect(record).To(BeNil()) + }) + }) ++ ++ Context("containerdisks checksum", func() { ++ var rootMountPoint string ++ ++ diskContent := []byte{0x6B, 0x75, 0x62, 0x65, 0x76, 0x69, 0x72, 0x74} ++ ++ BeforeEach(func() { ++ ctrl := gomock.NewController(GinkgoT()) ++ mockIsolationDetector := isolation.NewMockPodIsolationDetector(ctrl) ++ mockNodeIsolationResult := isolation.NewMockIsolationResult(ctrl) ++ mockPodIsolationResult := isolation.NewMockIsolationResult(ctrl) ++ ++ m.podIsolationDetector = mockIsolationDetector ++ m.nodeIsolationResult = mockNodeIsolationResult ++ ++ m.socketPathGetter = func(vmi *v1.VirtualMachineInstance, volumeIndex int) (string, error) { ++ return "somewhere", nil ++ } ++ m.kernelBootSocketPathGetter = func(vmi *v1.VirtualMachineInstance) (string, error) { ++ return "somewhere-kernel", nil ++ } ++ ++ mockIsolationDetector.EXPECT().DetectForSocket(gomock.Any(), gomock.Any()).Return(mockPodIsolationResult, nil) ++ ++ mockPodIsolationResult.EXPECT().Mounts(gomock.Any()).Return([]*mount.Info{&mount.Info{Root: "/", Mountpoint: "/disks"}}, nil) ++ ++ rootMountPoint, err = os.MkdirTemp(tmpDir, "root") ++ Expect(err).ToNot(HaveOccurred()) ++ partentToChildMountPoint, err := os.MkdirTemp(rootMountPoint, "child") ++ Expect(err).ToNot(HaveOccurred()) ++ mockNodeIsolationResult.EXPECT().Mounts(gomock.Any()).Return([]*mount.Info{&mount.Info{Root: partentToChildMountPoint}}, nil) ++ ++ rootMountPointSafePath, err := safepath.NewPathNoFollow(rootMountPoint) ++ Expect(err).ToNot(HaveOccurred()) ++ mockNodeIsolationResult.EXPECT().MountRoot().Return(rootMountPointSafePath, nil) ++ }) ++ ++ Context("verification", func() { ++ ++ type args struct { ++ storedChecksum uint32 ++ diskContent []byte ++ verifyMatcher gomega_types.GomegaMatcher ++ } ++ ++ DescribeTable(" should", func(args *args) { ++ vmiVolume := vmi.Spec.Volumes[0] ++ ++ err := os.WriteFile(filepath.Join(rootMountPoint, vmiVolume.ContainerDisk.Path), args.diskContent, 0660) ++ Expect(err).ToNot(HaveOccurred()) ++ ++ vmi.Status.VolumeStatus = []v1.VolumeStatus{ ++ v1.VolumeStatus{ ++ Name: vmiVolume.Name, ++ ContainerDiskVolume: &v1.ContainerDiskInfo{Checksum: args.storedChecksum}, ++ }, ++ } ++ ++ err = VerifyChecksums(m, vmi) ++ Expect(err).To(args.verifyMatcher) ++ ++ }, ++ Entry("succeed if source and target containerdisk match", &args{ ++ storedChecksum: crc32.ChecksumIEEE(diskContent), ++ diskContent: diskContent, ++ verifyMatcher: Not(HaveOccurred()), ++ }), ++ Entry("fail if checksum is not present", &args{ ++ storedChecksum: 0, ++ diskContent: diskContent, ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMissing)), ++ }), ++ Entry("fail if source and target containerdisk do not match", &args{ ++ storedChecksum: crc32.ChecksumIEEE([]byte{0xde, 0xad, 0xbe, 0xef}), ++ diskContent: diskContent, ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMismatch)), ++ }), ++ ) ++ }) ++ ++ Context("with custom kernel artifacts", func() { ++ ++ type args struct { ++ kernel []byte ++ initrd []byte ++ kernelChecksum uint32 ++ initrdChecksum uint32 ++ verifyMatcher gomega_types.GomegaMatcher ++ } ++ ++ DescribeTable("verification should", func(args *args) { ++ kernelBootVMI := api.NewMinimalVMI("fake-vmi") ++ kernelBootVMI.Spec.Domain.Firmware = &v1.Firmware{ ++ KernelBoot: &v1.KernelBoot{ ++ Container: &v1.KernelBootContainer{}, ++ }, ++ } ++ ++ if args.kernel != nil { ++ kernelFile, err := os.CreateTemp(rootMountPoint, kernelBootVMI.Spec.Domain.Firmware.KernelBoot.Container.KernelPath) ++ Expect(err).ToNot(HaveOccurred()) ++ defer kernelFile.Close() ++ ++ _, err = (kernelFile.Write(args.kernel)) ++ Expect(err).ToNot(HaveOccurred()) ++ ++ kernelBootVMI.Spec.Domain.Firmware.KernelBoot.Container.KernelPath = filepath.Join("/", filepath.Base(kernelFile.Name())) ++ } ++ ++ if args.initrd != nil { ++ initrdFile, err := os.CreateTemp(rootMountPoint, kernelBootVMI.Spec.Domain.Firmware.KernelBoot.Container.InitrdPath) ++ Expect(err).ToNot(HaveOccurred()) ++ defer initrdFile.Close() ++ ++ _, err = (initrdFile.Write(args.initrd)) ++ Expect(err).ToNot(HaveOccurred()) ++ ++ kernelBootVMI.Spec.Domain.Firmware.KernelBoot.Container.InitrdPath = filepath.Join("/", filepath.Base(initrdFile.Name())) ++ } ++ ++ kernelBootVMI.Status.KernelBootStatus = &v1.KernelBootStatus{} ++ if args.kernel != nil { ++ kernelBootVMI.Status.KernelBootStatus.KernelInfo = &v1.KernelInfo{Checksum: args.kernelChecksum} ++ } ++ if args.initrd != nil { ++ kernelBootVMI.Status.KernelBootStatus.InitrdInfo = &v1.InitrdInfo{Checksum: args.initrdChecksum} ++ } ++ ++ err = VerifyChecksums(m, kernelBootVMI) ++ Expect(err).To(args.verifyMatcher) ++ }, ++ Entry("succeed when source and target custom kernel match", &args{ ++ kernel: diskContent, ++ kernelChecksum: crc32.ChecksumIEEE(diskContent), ++ verifyMatcher: Not(HaveOccurred()), ++ }), ++ Entry("succeed when source and target custom initrd match", &args{ ++ initrd: diskContent, ++ initrdChecksum: crc32.ChecksumIEEE(diskContent), ++ verifyMatcher: Not(HaveOccurred()), ++ }), ++ Entry("succeed when source and target custom kernel and initrd match", &args{ ++ kernel: []byte{0xA, 0xB, 0xC, 0xD}, ++ kernelChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ initrd: []byte{0x1, 0x2, 0x3, 0x4}, ++ initrdChecksum: crc32.ChecksumIEEE([]byte{0x1, 0x2, 0x3, 0x4}), ++ verifyMatcher: Not(HaveOccurred()), ++ }), ++ Entry("fail when source and target custom kernel do not match", &args{ ++ kernel: diskContent, ++ kernelChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMismatch)), ++ }), ++ Entry("fail when source and target custom initrd do not match", &args{ ++ initrd: diskContent, ++ initrdChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMismatch)), ++ }), ++ Entry("fail when checksum is missing", &args{ ++ kernel: diskContent, ++ initrd: diskContent, ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMissing)), ++ }), ++ Entry("fail when source and target custom kernel match but initrd does not", &args{ ++ kernel: []byte{0xA, 0xB, 0xC, 0xD}, ++ kernelChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ initrd: []byte{0xF, 0xF, 0xE, 0xE}, ++ initrdChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMismatch)), ++ }), ++ Entry("fail when source and target custom initrd match but kernel does not", &args{ ++ kernel: []byte{0xF, 0xF, 0xE, 0xE}, ++ kernelChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ initrd: []byte{0x1, 0x2, 0x3, 0x4}, ++ initrdChecksum: crc32.ChecksumIEEE([]byte{0x1, 0x2, 0x3, 0x4}), ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMismatch)), ++ }), ++ Entry("fail when source and target custom initrd and kernel do not match", &args{ ++ kernel: []byte{0xF, 0xF, 0xE, 0xE}, ++ kernelChecksum: crc32.ChecksumIEEE([]byte{0xA, 0xB, 0xC, 0xD}), ++ initrd: []byte{0xA, 0xB, 0xC, 0xD}, ++ initrdChecksum: crc32.ChecksumIEEE([]byte{0x1, 0x2, 0x3, 0x4}), ++ verifyMatcher: And(HaveOccurred(), MatchError(ErrChecksumMismatch)), ++ }), ++ ) ++ }) ++ }) + }) diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08 new file mode 100644 index 0000000000..c0448602ba --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch08 @@ -0,0 +1,26 @@ +a/pkg/virt-handler/isolation/isolation.go b/pkg/virt-handler/isolation/isolation.go +index f475042c2..1eb8b0f4e 100644 +--- a/pkg/virt-handler/isolation/isolation.go ++++ b/pkg/virt-handler/isolation/isolation.go +@@ -105,21 +105,6 @@ func IsMounted(mountPoint *safepath.Path) (isMounted bool, err error) { + } + } + +-// AreMounted checks if given paths are mounted by calling IsMounted. +-// If error occurs, the first error is returned. +-func (r *RealIsolationResult) AreMounted(mountPoints ...*safepath.Path) (isMounted bool, err error) { +- for _, mountPoint := range mountPoints { +- if mountPoint != nil { +- isMounted, err = IsMounted(mountPoint) +- if !isMounted || err != nil { +- return +- } +- } +- } +- +- return true, nil +-} +- + // IsBlockDevice checks if the given path is a block device or not. + func IsBlockDevice(path *safepath.Path) (bool, error) { + fileInfo, err := safepath.StatAtNoFollow(path) diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09 new file mode 100644 index 0000000000..d08bd4c052 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch09 @@ -0,0 +1,137 @@ +a/pkg/virt-handler/vm.go b/pkg/virt-handler/vm.go +index 82eb6ef6c..da69acfaa 100644 +--- a/pkg/virt-handler/vm.go ++++ b/pkg/virt-handler/vm.go +@@ -888,6 +888,99 @@ func (d *VirtualMachineController) updateHotplugVolumeStatus(vmi *v1.VirtualMach + return volumeStatus, needsRefresh + } + ++func needToComputeChecksums(vmi *v1.VirtualMachineInstance) bool { ++ containerDisks := map[string]*v1.Volume{} ++ for _, volume := range vmi.Spec.Volumes { ++ if volume.VolumeSource.ContainerDisk != nil { ++ containerDisks[volume.Name] = &volume ++ } ++ } ++ ++ for i := range vmi.Status.VolumeStatus { ++ _, isContainerDisk := containerDisks[vmi.Status.VolumeStatus[i].Name] ++ if !isContainerDisk { ++ continue ++ } ++ ++ if vmi.Status.VolumeStatus[i].ContainerDiskVolume == nil || ++ vmi.Status.VolumeStatus[i].ContainerDiskVolume.Checksum == 0 { ++ return true ++ } ++ } ++ ++ if util.HasKernelBootContainerImage(vmi) { ++ if vmi.Status.KernelBootStatus == nil { ++ return true ++ } ++ ++ kernelBootContainer := vmi.Spec.Domain.Firmware.KernelBoot.Container ++ ++ if kernelBootContainer.KernelPath != "" && ++ (vmi.Status.KernelBootStatus.KernelInfo == nil || ++ vmi.Status.KernelBootStatus.KernelInfo.Checksum == 0) { ++ return true ++ ++ } ++ ++ if kernelBootContainer.InitrdPath != "" && ++ (vmi.Status.KernelBootStatus.InitrdInfo == nil || ++ vmi.Status.KernelBootStatus.InitrdInfo.Checksum == 0) { ++ return true ++ ++ } ++ } ++ ++ return false ++} ++ ++func (d *VirtualMachineController) updateChecksumInfo(vmi *v1.VirtualMachineInstance, syncError error) error { ++ ++ if syncError != nil || vmi.DeletionTimestamp != nil || !needToComputeChecksums(vmi) { ++ return nil ++ } ++ ++ diskChecksums, err := d.containerDiskMounter.ComputeChecksums(vmi) ++ if goerror.Is(err, container_disk.ErrDiskContainerGone) { ++ log.Log.Errorf("cannot compute checksums as containerdisk/kernelboot containers seem to have been terminated") ++ return nil ++ } ++ if err != nil { ++ return err ++ } ++ ++ // containerdisks ++ for i := range vmi.Status.VolumeStatus { ++ checksum, exists := diskChecksums.ContainerDiskChecksums[vmi.Status.VolumeStatus[i].Name] ++ if !exists { ++ // not a containerdisk ++ continue ++ } ++ ++ vmi.Status.VolumeStatus[i].ContainerDiskVolume = &v1.ContainerDiskInfo{ ++ Checksum: checksum, ++ } ++ } ++ ++ // kernelboot ++ if util.HasKernelBootContainerImage(vmi) { ++ vmi.Status.KernelBootStatus = &v1.KernelBootStatus{} ++ ++ if diskChecksums.KernelBootChecksum.Kernel != nil { ++ vmi.Status.KernelBootStatus.KernelInfo = &v1.KernelInfo{ ++ Checksum: *diskChecksums.KernelBootChecksum.Kernel, ++ } ++ } ++ ++ if diskChecksums.KernelBootChecksum.Initrd != nil { ++ vmi.Status.KernelBootStatus.InitrdInfo = &v1.InitrdInfo{ ++ Checksum: *diskChecksums.KernelBootChecksum.Initrd, ++ } ++ } ++ } ++ ++ return nil ++} ++ + func (d *VirtualMachineController) updateVolumeStatusesFromDomain(vmi *v1.VirtualMachineInstance, domain *api.Domain) bool { + // used by unit test + hasHotplug := false +@@ -1338,6 +1431,11 @@ func (d *VirtualMachineController) updateVMIStatus(origVMI *v1.VirtualMachineIns + return err + } + ++ // Store containerdisks and kernelboot checksums ++ if err := d.updateChecksumInfo(vmi, syncError); err != nil { ++ return err ++ } ++ + // Handle sync error + handleSyncError(vmi, condManager, syncError) + +@@ -2683,6 +2781,20 @@ func (d *VirtualMachineController) vmUpdateHelperMigrationTarget(origVMI *v1.Vir + return nil + } + ++ // Verify container disks checksum ++ err = container_disk.VerifyChecksums(d.containerDiskMounter, vmi) ++ switch { ++ case goerror.Is(err, container_disk.ErrChecksumMissing): ++ // wait for checksum to be computed by the source virt-handler ++ return err ++ case goerror.Is(err, container_disk.ErrChecksumMismatch): ++ log.Log.Object(vmi).Infof("Containerdisk checksum mismatch, terminating target pod: %s", err) ++ d.recorder.Event(vmi, k8sv1.EventTypeNormal, "ContainerDiskFailedChecksum", "Aborting migration as the source and target containerdisks/kernelboot do not match") ++ return client.SignalTargetPodCleanup(vmi) ++ case err != nil: ++ return err ++ } ++ + // Mount container disks + disksInfo, err := d.containerDiskMounter.MountAndVerify(vmi) + if err != nil { diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10 new file mode 100644 index 0000000000..8d1584fea4 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch10 @@ -0,0 +1,70 @@ +a/pkg/virt-handler/vm_test.go b/pkg/virt-handler/vm_test.go +index 273d3f30c..f0ff6d394 100644 +--- a/pkg/virt-handler/vm_test.go ++++ b/pkg/virt-handler/vm_test.go +@@ -1294,6 +1294,65 @@ var _ = Describe("VirtualMachineInstance", func() { + Expect(mockQueue.Len()).To(Equal(0)) + Expect(mockQueue.GetRateLimitedEnqueueCount()).To(Equal(1)) + }) ++ ++ It("should compute checksums for the specified containerDisks and kernelboot containers", func() { ++ vmi := NewScheduledVMIWithContainerDisk(vmiTestUUID, podTestUUID, host) ++ vmi.Status.Phase = v1.Running ++ vmi.Status.VolumeStatus = []v1.VolumeStatus{ ++ v1.VolumeStatus{ ++ Name: vmi.Spec.Volumes[0].Name, ++ }, ++ } ++ vmi.Spec.Domain.Firmware = &v1.Firmware{ ++ KernelBoot: &v1.KernelBoot{ ++ Container: &v1.KernelBootContainer{ ++ KernelPath: "/vmlinuz", ++ InitrdPath: "/initrd", ++ }, ++ }, ++ } ++ ++ domain := api.NewMinimalDomainWithUUID("testvmi", vmiTestUUID) ++ domain.Status.Status = api.Running ++ domainFeeder.Add(domain) ++ ++ mockWatchdog.CreateFile(vmi) ++ vmiFeeder.Add(vmi) ++ ++ fakeDiskChecksums := &container_disk.DiskChecksums{ ++ ContainerDiskChecksums: map[string]uint32{ ++ vmi.Spec.Volumes[0].Name: uint32(1234), ++ }, ++ KernelBootChecksum: container_disk.KernelBootChecksum{ ++ Kernel: pointer.Uint32(33), ++ Initrd: pointer.Uint32(35), ++ }, ++ } ++ ++ mockHotplugVolumeMounter.EXPECT().Mount(gomock.Any(), gomock.Any()).Return(nil) ++ mockContainerDiskMounter.EXPECT().ComputeChecksums(gomock.Any()).Return(fakeDiskChecksums, nil) ++ client.EXPECT().SyncVirtualMachine(gomock.Any(), gomock.Any()).Return(nil) ++ mockHotplugVolumeMounter.EXPECT().Unmount(gomock.Any(), gomock.Any()).Return(nil) ++ ++ vmiInterface.EXPECT().Update(context.Background(), gomock.Any()).DoAndReturn( ++ func(ctx context.Context, vmi *v1.VirtualMachineInstance) (*v1.VirtualMachineInstance, error) { ++ diskChecksum := fakeDiskChecksums.ContainerDiskChecksums[vmi.Status.VolumeStatus[0].Name] ++ kernelChecksum := *fakeDiskChecksums.KernelBootChecksum.Kernel ++ initrdChecksum := *fakeDiskChecksums.KernelBootChecksum.Initrd ++ ++ Expect(vmi.Status.VolumeStatus[0].ContainerDiskVolume).ToNot(BeNil()) ++ Expect(vmi.Status.VolumeStatus[0].ContainerDiskVolume.Checksum).To(Equal(diskChecksum)) ++ ++ Expect(vmi.Status.KernelBootStatus).ToNot(BeNil()) ++ Expect(vmi.Status.KernelBootStatus.KernelInfo).ToNot(BeNil()) ++ Expect(vmi.Status.KernelBootStatus.InitrdInfo).ToNot(BeNil()) ++ Expect(vmi.Status.KernelBootStatus.KernelInfo.Checksum).To(Equal(kernelChecksum)) ++ Expect(vmi.Status.KernelBootStatus.InitrdInfo.Checksum).To(Equal(initrdChecksum)) ++ return vmi, nil ++ }) ++ ++ controller.Execute() ++ }) + }) + + Context("reacting to a VMI with hotplug", func() { diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11 new file mode 100644 index 0000000000..e7d942ea7c --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch11 @@ -0,0 +1,17 @@ +a/pkg/virt-operator/resource/generate/components/daemonsets.go b/pkg/virt-operator/resource/generate/components/daemonsets.go +index da66b6cd9..aebc64605 100644 +--- a/pkg/virt-operator/resource/generate/components/daemonsets.go ++++ b/pkg/virt-operator/resource/generate/components/daemonsets.go +@@ -320,8 +320,10 @@ func NewHandlerDaemonSet(namespace, repository, imagePrefix, version, launcherVe + + container.Resources = corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ +- corev1.ResourceCPU: resource.MustParse("10m"), +- corev1.ResourceMemory: resource.MustParse("325Mi"), ++ corev1.ResourceCPU: resource.MustParse("10m"), ++ // 325Mi - base memory request ++ // +32Mi - to account for the buffer used to verify containerdisk checksums ++ corev1.ResourceMemory: resource.MustParse("357Mi"), + }, + } + if prHelperImage == "" { diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12 new file mode 100644 index 0000000000..baa51953a1 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch12 @@ -0,0 +1,48 @@ +a/pkg/virt-operator/resource/generate/components/validations_generated.go b/pkg/virt-operator/resource/generate/components/validations_generated.go +index cd0fb9fe0..b23633e34 100644 +--- a/pkg/virt-operator/resource/generate/components/validations_generated.go ++++ b/pkg/virt-operator/resource/generate/components/validations_generated.go +@@ -12051,6 +12051,26 @@ var CRDsValidation map[string]string = map[string]string{ + type: integer + type: object + type: array ++ kernelBootStatus: ++ description: KernelBootStatus contains info about the kernelBootContainer ++ properties: ++ initrdInfo: ++ description: InitrdInfo show info about the initrd file ++ properties: ++ checksum: ++ description: Checksum is the checksum of the initrd file ++ format: int32 ++ type: integer ++ type: object ++ kernelInfo: ++ description: KernelInfo show info about the kernel image ++ properties: ++ checksum: ++ description: Checksum is the checksum of the kernel image ++ format: int32 ++ type: integer ++ type: object ++ type: object + launcherContainerImageVersion: + description: LauncherContainerImageVersion indicates what container image + is currently active for the vmi. +@@ -12322,6 +12342,16 @@ var CRDsValidation map[string]string = map[string]string{ + description: VolumeStatus represents information about the status of volumes + attached to the VirtualMachineInstance. + properties: ++ containerDiskVolume: ++ description: ContainerDiskVolume shows info about the containerdisk, ++ if the volume is a containerdisk ++ properties: ++ checksum: ++ description: Checksum is the checksum of the rootdisk or kernel ++ artifacts inside the containerdisk ++ format: int32 ++ type: integer ++ type: object + hotplugVolume: + description: If the volume is hotplug, this will contain the hotplug + status. diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13 new file mode 100644 index 0000000000..2836621705 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch13 @@ -0,0 +1,123 @@ +a/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go b/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go +index 16f004695..425940926 100644 +--- a/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go ++++ b/staging/src/kubevirt.io/api/core/v1/deepcopy_generated.go +@@ -609,6 +609,22 @@ func (in *ConfigMapVolumeSource) DeepCopy() *ConfigMapVolumeSource { + return out + } + ++// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. ++func (in *ContainerDiskInfo) DeepCopyInto(out *ContainerDiskInfo) { ++ *out = *in ++ return ++} ++ ++// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ContainerDiskInfo. ++func (in *ContainerDiskInfo) DeepCopy() *ContainerDiskInfo { ++ if in == nil { ++ return nil ++ } ++ out := new(ContainerDiskInfo) ++ in.DeepCopyInto(out) ++ return out ++} ++ + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. + func (in *ContainerDiskSource) DeepCopyInto(out *ContainerDiskSource) { + *out = *in +@@ -1940,6 +1956,22 @@ func (in *I6300ESBWatchdog) DeepCopy() *I6300ESBWatchdog { + return out + } + ++// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. ++func (in *InitrdInfo) DeepCopyInto(out *InitrdInfo) { ++ *out = *in ++ return ++} ++ ++// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new InitrdInfo. ++func (in *InitrdInfo) DeepCopy() *InitrdInfo { ++ if in == nil { ++ return nil ++ } ++ out := new(InitrdInfo) ++ in.DeepCopyInto(out) ++ return out ++} ++ + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. + func (in *Input) DeepCopyInto(out *Input) { + *out = *in +@@ -2251,6 +2283,48 @@ func (in *KernelBootContainer) DeepCopy() *KernelBootContainer { + return out + } + ++// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. ++func (in *KernelBootStatus) DeepCopyInto(out *KernelBootStatus) { ++ *out = *in ++ if in.KernelInfo != nil { ++ in, out := &in.KernelInfo, &out.KernelInfo ++ *out = new(KernelInfo) ++ **out = **in ++ } ++ if in.InitrdInfo != nil { ++ in, out := &in.InitrdInfo, &out.InitrdInfo ++ *out = new(InitrdInfo) ++ **out = **in ++ } ++ return ++} ++ ++// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new KernelBootStatus. ++func (in *KernelBootStatus) DeepCopy() *KernelBootStatus { ++ if in == nil { ++ return nil ++ } ++ out := new(KernelBootStatus) ++ in.DeepCopyInto(out) ++ return out ++} ++ ++// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. ++func (in *KernelInfo) DeepCopyInto(out *KernelInfo) { ++ *out = *in ++ return ++} ++ ++// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new KernelInfo. ++func (in *KernelInfo) DeepCopy() *KernelInfo { ++ if in == nil { ++ return nil ++ } ++ out := new(KernelInfo) ++ in.DeepCopyInto(out) ++ return out ++} ++ + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. + func (in *KubeVirt) DeepCopyInto(out *KubeVirt) { + *out = *in +@@ -5447,6 +5521,11 @@ func (in *VirtualMachineInstanceStatus) DeepCopyInto(out *VirtualMachineInstance + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } ++ if in.KernelBootStatus != nil { ++ in, out := &in.KernelBootStatus, &out.KernelBootStatus ++ *out = new(KernelBootStatus) ++ (*in).DeepCopyInto(*out) ++ } + if in.TopologyHints != nil { + in, out := &in.TopologyHints, &out.TopologyHints + *out = new(TopologyHints) +@@ -5922,6 +6001,11 @@ func (in *VolumeStatus) DeepCopyInto(out *VolumeStatus) { + *out = new(DomainMemoryDumpInfo) + (*in).DeepCopyInto(*out) + } ++ if in.ContainerDiskVolume != nil { ++ in, out := &in.ContainerDiskVolume, &out.ContainerDiskVolume ++ *out = new(ContainerDiskInfo) ++ **out = **in ++ } + return + } + diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14 new file mode 100644 index 0000000000..41bd7cb9d2 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14 @@ -0,0 +1,57 @@ +a/staging/src/kubevirt.io/api/core/v1/types.go b/staging/src/kubevirt.io/api/core/v1/types.go +index 30da6f6bc..4d7266f8e 100644 +--- a/staging/src/kubevirt.io/api/core/v1/types.go ++++ b/staging/src/kubevirt.io/api/core/v1/types.go +@@ -247,6 +247,10 @@ type VirtualMachineInstanceStatus struct { + // +listType=atomic + VolumeStatus []VolumeStatus `json:"volumeStatus,omitempty"` + ++ // KernelBootStatus contains info about the kernelBootContainer ++ // +optional ++ KernelBootStatus *KernelBootStatus `json:"kernelBootStatus,omitempty"` ++ + // FSFreezeStatus is the state of the fs of the guest + // it can be either frozen or thawed + // +optional +@@ -336,6 +340,28 @@ type VolumeStatus struct { + Size int64 `json:"size,omitempty"` + // If the volume is memorydump volume, this will contain the memorydump info. + MemoryDumpVolume *DomainMemoryDumpInfo `json:"memoryDumpVolume,omitempty"` ++ // ContainerDiskVolume shows info about the containerdisk, if the volume is a containerdisk ++ ContainerDiskVolume *ContainerDiskInfo `json:"containerDiskVolume,omitempty"` ++} ++ ++// KernelInfo show info about the kernel image ++type KernelInfo struct { ++ // Checksum is the checksum of the kernel image ++ Checksum uint32 `json:"checksum,omitempty"` ++} ++ ++// InitrdInfo show info about the initrd file ++type InitrdInfo struct { ++ // Checksum is the checksum of the initrd file ++ Checksum uint32 `json:"checksum,omitempty"` ++} ++ ++// KernelBootStatus contains info about the kernelBootContainer ++type KernelBootStatus struct { ++ // KernelInfo show info about the kernel image ++ KernelInfo *KernelInfo `json:"kernelInfo,omitempty"` ++ // InitrdInfo show info about the initrd file ++ InitrdInfo *InitrdInfo `json:"initrdInfo,omitempty"` + } + + // DomainMemoryDumpInfo represents the memory dump information +@@ -358,6 +384,12 @@ type HotplugVolumeStatus struct { + AttachPodUID types.UID `json:"attachPodUID,omitempty"` + } + ++// ContainerDiskInfo shows info about the containerdisk ++type ContainerDiskInfo struct { ++ // Checksum is the checksum of the rootdisk or kernel artifacts inside the containerdisk ++ Checksum uint32 `json:"checksum,omitempty"` ++} ++ + // VolumePhase indicates the current phase of the hotplug process. + type VolumePhase string + diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15 new file mode 100644 index 0000000000..32c6294a2e --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15 @@ -0,0 +1,56 @@ +a/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go b/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go +index 213228f96..ff3fbadb4 100644 +--- a/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go ++++ b/staging/src/kubevirt.io/api/core/v1/types_swagger_generated.go +@@ -72,6 +72,7 @@ func (VirtualMachineInstanceStatus) SwaggerDoc() map[string]string { + "evacuationNodeName": "EvacuationNodeName is used to track the eviction process of a VMI. It stores the name of the node that we want\nto evacuate. It is meant to be used by KubeVirt core components only and can't be set or modified by users.\n+optional", + "activePods": "ActivePods is a mapping of pod UID to node name.\nIt is possible for multiple pods to be running for a single VMI during migration.", + "volumeStatus": "VolumeStatus contains the statuses of all the volumes\n+optional\n+listType=atomic", ++ "kernelBootStatus": "KernelBootStatus contains info about the kernelBootContainer\n+optional", + "fsFreezeStatus": "FSFreezeStatus is the state of the fs of the guest\nit can be either frozen or thawed\n+optional", + "topologyHints": "+optional", + "virtualMachineRevisionName": "VirtualMachineRevisionName is used to get the vm revision of the vmi when doing\nan online vm snapshot\n+optional", +@@ -108,6 +109,29 @@ func (VolumeStatus) SwaggerDoc() map[string]string { + "hotplugVolume": "If the volume is hotplug, this will contain the hotplug status.", + "size": "Represents the size of the volume", + "memoryDumpVolume": "If the volume is memorydump volume, this will contain the memorydump info.", ++ "containerDiskVolume": "ContainerDiskVolume shows info about the containerdisk, if the volume is a containerdisk", ++ } ++} ++ ++func (KernelInfo) SwaggerDoc() map[string]string { ++ return map[string]string{ ++ "": "KernelInfo show info about the kernel image", ++ "checksum": "Checksum is the checksum of the kernel image", ++ } ++} ++ ++func (InitrdInfo) SwaggerDoc() map[string]string { ++ return map[string]string{ ++ "": "InitrdInfo show info about the initrd file", ++ "checksum": "Checksum is the checksum of the initrd file", ++ } ++} ++ ++func (KernelBootStatus) SwaggerDoc() map[string]string { ++ return map[string]string{ ++ "": "KernelBootStatus contains info about the kernelBootContainer", ++ "kernelInfo": "KernelInfo show info about the kernel image", ++ "initrdInfo": "InitrdInfo show info about the initrd file", + } + } + +@@ -129,6 +153,13 @@ func (HotplugVolumeStatus) SwaggerDoc() map[string]string { + } + } + ++func (ContainerDiskInfo) SwaggerDoc() map[string]string { ++ return map[string]string{ ++ "": "ContainerDiskInfo shows info about the containerdisk", ++ "checksum": "Checksum is the checksum of the rootdisk or kernel artifacts inside the containerdisk", ++ } ++} ++ + func (VirtualMachineInstanceCondition) SwaggerDoc() map[string]string { + return map[string]string{ + "lastProbeTime": "+nullable", diff --git a/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16 b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16 new file mode 100644 index 0000000000..a77814fc32 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16 @@ -0,0 +1,179 @@ +a/staging/src/kubevirt.io/client-go/api/openapi_generated.go b/staging/src/kubevirt.io/client-go/api/openapi_generated.go +index 06eef420f..82ef9cb76 100644 +--- a/staging/src/kubevirt.io/client-go/api/openapi_generated.go ++++ b/staging/src/kubevirt.io/client-go/api/openapi_generated.go +@@ -332,6 +332,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA + "kubevirt.io/api/core/v1.ComponentConfig": schema_kubevirtio_api_core_v1_ComponentConfig(ref), + "kubevirt.io/api/core/v1.ConfigDriveSSHPublicKeyAccessCredentialPropagation": schema_kubevirtio_api_core_v1_ConfigDriveSSHPublicKeyAccessCredentialPropagation(ref), + "kubevirt.io/api/core/v1.ConfigMapVolumeSource": schema_kubevirtio_api_core_v1_ConfigMapVolumeSource(ref), ++ "kubevirt.io/api/core/v1.ContainerDiskInfo": schema_kubevirtio_api_core_v1_ContainerDiskInfo(ref), + "kubevirt.io/api/core/v1.ContainerDiskSource": schema_kubevirtio_api_core_v1_ContainerDiskSource(ref), + "kubevirt.io/api/core/v1.CustomBlockSize": schema_kubevirtio_api_core_v1_CustomBlockSize(ref), + "kubevirt.io/api/core/v1.CustomProfile": schema_kubevirtio_api_core_v1_CustomProfile(ref), +@@ -383,6 +384,7 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA + "kubevirt.io/api/core/v1.Hugepages": schema_kubevirtio_api_core_v1_Hugepages(ref), + "kubevirt.io/api/core/v1.HypervTimer": schema_kubevirtio_api_core_v1_HypervTimer(ref), + "kubevirt.io/api/core/v1.I6300ESBWatchdog": schema_kubevirtio_api_core_v1_I6300ESBWatchdog(ref), ++ "kubevirt.io/api/core/v1.InitrdInfo": schema_kubevirtio_api_core_v1_InitrdInfo(ref), + "kubevirt.io/api/core/v1.Input": schema_kubevirtio_api_core_v1_Input(ref), + "kubevirt.io/api/core/v1.InstancetypeMatcher": schema_kubevirtio_api_core_v1_InstancetypeMatcher(ref), + "kubevirt.io/api/core/v1.Interface": schema_kubevirtio_api_core_v1_Interface(ref), +@@ -398,6 +400,8 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA + "kubevirt.io/api/core/v1.KVMTimer": schema_kubevirtio_api_core_v1_KVMTimer(ref), + "kubevirt.io/api/core/v1.KernelBoot": schema_kubevirtio_api_core_v1_KernelBoot(ref), + "kubevirt.io/api/core/v1.KernelBootContainer": schema_kubevirtio_api_core_v1_KernelBootContainer(ref), ++ "kubevirt.io/api/core/v1.KernelBootStatus": schema_kubevirtio_api_core_v1_KernelBootStatus(ref), ++ "kubevirt.io/api/core/v1.KernelInfo": schema_kubevirtio_api_core_v1_KernelInfo(ref), + "kubevirt.io/api/core/v1.KubeVirt": schema_kubevirtio_api_core_v1_KubeVirt(ref), + "kubevirt.io/api/core/v1.KubeVirtCertificateRotateStrategy": schema_kubevirtio_api_core_v1_KubeVirtCertificateRotateStrategy(ref), + "kubevirt.io/api/core/v1.KubeVirtCondition": schema_kubevirtio_api_core_v1_KubeVirtCondition(ref), +@@ -16337,6 +16341,26 @@ func schema_kubevirtio_api_core_v1_ConfigMapVolumeSource(ref common.ReferenceCal + } + } + ++func schema_kubevirtio_api_core_v1_ContainerDiskInfo(ref common.ReferenceCallback) common.OpenAPIDefinition { ++ return common.OpenAPIDefinition{ ++ Schema: spec.Schema{ ++ SchemaProps: spec.SchemaProps{ ++ Description: "ContainerDiskInfo shows info about the containerdisk", ++ Type: []string{"object"}, ++ Properties: map[string]spec.Schema{ ++ "checksum": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "Checksum is the checksum of the rootdisk or kernel artifacts inside the containerdisk", ++ Type: []string{"integer"}, ++ Format: "int64", ++ }, ++ }, ++ }, ++ }, ++ }, ++ } ++} ++ + func schema_kubevirtio_api_core_v1_ContainerDiskSource(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ +@@ -18296,6 +18320,26 @@ func schema_kubevirtio_api_core_v1_I6300ESBWatchdog(ref common.ReferenceCallback + } + } + ++func schema_kubevirtio_api_core_v1_InitrdInfo(ref common.ReferenceCallback) common.OpenAPIDefinition { ++ return common.OpenAPIDefinition{ ++ Schema: spec.Schema{ ++ SchemaProps: spec.SchemaProps{ ++ Description: "InitrdInfo show info about the initrd file", ++ Type: []string{"object"}, ++ Properties: map[string]spec.Schema{ ++ "checksum": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "Checksum is the checksum of the initrd file", ++ Type: []string{"integer"}, ++ Format: "int64", ++ }, ++ }, ++ }, ++ }, ++ }, ++ } ++} ++ + func schema_kubevirtio_api_core_v1_Input(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ +@@ -18764,6 +18808,53 @@ func schema_kubevirtio_api_core_v1_KernelBootContainer(ref common.ReferenceCallb + } + } + ++func schema_kubevirtio_api_core_v1_KernelBootStatus(ref common.ReferenceCallback) common.OpenAPIDefinition { ++ return common.OpenAPIDefinition{ ++ Schema: spec.Schema{ ++ SchemaProps: spec.SchemaProps{ ++ Description: "KernelBootStatus contains info about the kernelBootContainer", ++ Type: []string{"object"}, ++ Properties: map[string]spec.Schema{ ++ "kernelInfo": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "KernelInfo show info about the kernel image", ++ Ref: ref("kubevirt.io/api/core/v1.KernelInfo"), ++ }, ++ }, ++ "initrdInfo": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "InitrdInfo show info about the initrd file", ++ Ref: ref("kubevirt.io/api/core/v1.InitrdInfo"), ++ }, ++ }, ++ }, ++ }, ++ }, ++ Dependencies: []string{ ++ "kubevirt.io/api/core/v1.InitrdInfo", "kubevirt.io/api/core/v1.KernelInfo"}, ++ } ++} ++ ++func schema_kubevirtio_api_core_v1_KernelInfo(ref common.ReferenceCallback) common.OpenAPIDefinition { ++ return common.OpenAPIDefinition{ ++ Schema: spec.Schema{ ++ SchemaProps: spec.SchemaProps{ ++ Description: "KernelInfo show info about the kernel image", ++ Type: []string{"object"}, ++ Properties: map[string]spec.Schema{ ++ "checksum": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "Checksum is the checksum of the kernel image", ++ Type: []string{"integer"}, ++ Format: "int64", ++ }, ++ }, ++ }, ++ }, ++ }, ++ } ++} ++ + func schema_kubevirtio_api_core_v1_KubeVirt(ref common.ReferenceCallback) common.OpenAPIDefinition { + return common.OpenAPIDefinition{ + Schema: spec.Schema{ +@@ -24313,6 +24404,12 @@ func schema_kubevirtio_api_core_v1_VirtualMachineInstanceStatus(ref common.Refer + }, + }, + }, ++ "kernelBootStatus": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "KernelBootStatus contains info about the kernelBootContainer", ++ Ref: ref("kubevirt.io/api/core/v1.KernelBootStatus"), ++ }, ++ }, + "fsFreezeStatus": { + SchemaProps: spec.SchemaProps{ + Description: "FSFreezeStatus is the state of the fs of the guest it can be either frozen or thawed", +@@ -24376,7 +24473,7 @@ func schema_kubevirtio_api_core_v1_VirtualMachineInstanceStatus(ref common.Refer + }, + }, + Dependencies: []string{ +- "kubevirt.io/api/core/v1.CPUTopology", "kubevirt.io/api/core/v1.Machine", "kubevirt.io/api/core/v1.MemoryStatus", "kubevirt.io/api/core/v1.TopologyHints", "kubevirt.io/api/core/v1.VirtualMachineInstanceCondition", "kubevirt.io/api/core/v1.VirtualMachineInstanceGuestOSInfo", "kubevirt.io/api/core/v1.VirtualMachineInstanceMigrationState", "kubevirt.io/api/core/v1.VirtualMachineInstanceNetworkInterface", "kubevirt.io/api/core/v1.VirtualMachineInstancePhaseTransitionTimestamp", "kubevirt.io/api/core/v1.VolumeStatus"}, ++ "kubevirt.io/api/core/v1.CPUTopology", "kubevirt.io/api/core/v1.KernelBootStatus", "kubevirt.io/api/core/v1.Machine", "kubevirt.io/api/core/v1.MemoryStatus", "kubevirt.io/api/core/v1.TopologyHints", "kubevirt.io/api/core/v1.VirtualMachineInstanceCondition", "kubevirt.io/api/core/v1.VirtualMachineInstanceGuestOSInfo", "kubevirt.io/api/core/v1.VirtualMachineInstanceMigrationState", "kubevirt.io/api/core/v1.VirtualMachineInstanceNetworkInterface", "kubevirt.io/api/core/v1.VirtualMachineInstancePhaseTransitionTimestamp", "kubevirt.io/api/core/v1.VolumeStatus"}, + } + } + +@@ -25179,12 +25276,18 @@ func schema_kubevirtio_api_core_v1_VolumeStatus(ref common.ReferenceCallback) co + Ref: ref("kubevirt.io/api/core/v1.DomainMemoryDumpInfo"), + }, + }, ++ "containerDiskVolume": { ++ SchemaProps: spec.SchemaProps{ ++ Description: "ContainerDiskVolume shows info about the containerdisk, if the volume is a containerdisk", ++ Ref: ref("kubevirt.io/api/core/v1.ContainerDiskInfo"), ++ }, ++ }, + }, + Required: []string{"name", "target"}, + }, + }, + Dependencies: []string{ +- "kubevirt.io/api/core/v1.DomainMemoryDumpInfo", "kubevirt.io/api/core/v1.HotplugVolumeStatus", "kubevirt.io/api/core/v1.PersistentVolumeClaimInfo"}, ++ "kubevirt.io/api/core/v1.ContainerDiskInfo", "kubevirt.io/api/core/v1.DomainMemoryDumpInfo", "kubevirt.io/api/core/v1.HotplugVolumeStatus", "kubevirt.io/api/core/v1.PersistentVolumeClaimInfo"}, + } + } + From 4b084271634f52d2e0e0beab4faacae8083cc57c Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Tue, 12 Dec 2023 19:00:13 +0100 Subject: [PATCH 2/8] botreview, test: make AttackReviewComment testable Introduce interfaces for faking input and output, create some basic test cases to fix behavior. Signed-off-by: Daniel Hiller --- external-plugins/botreview/review/review.go | 7 +- .../botreview/review/review_test.go | 181 ++++++++++++++++++ 2 files changed, 187 insertions(+), 1 deletion(-) diff --git a/external-plugins/botreview/review/review.go b/external-plugins/botreview/review/review.go index add9e6368f..81e55c7a43 100644 --- a/external-plugins/botreview/review/review.go +++ b/external-plugins/botreview/review/review.go @@ -154,7 +154,12 @@ const approvePRComment = `This PR satisfies all automated review criteria. /approve` const unapprovePRComment = "This PR does not satisfy at least one automated review criteria." -func (r *Reviewer) AttachReviewComments(botReviewResults []BotReviewResult, githubClient github.Client) error { +type gitHubReviewClient interface { + CreateComment(org, repo string, number int, comment string) error + BotUser() (*github.UserData, error) +} + +func (r *Reviewer) AttachReviewComments(botReviewResults []BotReviewResult, githubClient gitHubReviewClient) error { botUser, err := githubClient.BotUser() if err != nil { return fmt.Errorf("error while fetching user data: %v", err) diff --git a/external-plugins/botreview/review/review_test.go b/external-plugins/botreview/review/review_test.go index 08a02b20ed..4a312f2504 100644 --- a/external-plugins/botreview/review/review_test.go +++ b/external-plugins/botreview/review/review_test.go @@ -19,7 +19,10 @@ package review import ( + "encoding/json" + "github.com/sirupsen/logrus" "github.com/sourcegraph/go-diff/diff" + "k8s.io/test-infra/prow/github" "os" "reflect" "testing" @@ -174,3 +177,181 @@ func TestGuessReviewTypes(t *testing.T) { }) } } + +func TestReviewer_AttachReviewComments(t *testing.T) { + type fields struct { + l *logrus.Entry + org string + repo string + num int + user string + action github.PullRequestEventAction + dryRun bool + BaseSHA string + } + type args struct { + botReviewResults []BotReviewResult + githubClient FakeGHReviewClient + } + tests := []struct { + name string + fields fields + args args + wantErr bool + wantReviewComments []*FakeComment + }{ + { + name: "basic comment", + fields: fields{ + l: newEntry(), + org: "", + repo: "", + num: 0, + user: "", + action: "", + dryRun: false, + BaseSHA: "", + }, + args: args{ + githubClient: newGHReviewClient(), + botReviewResults: []BotReviewResult{}, + }, + wantErr: false, + wantReviewComments: []*FakeComment{ + { + Org: "", + Repo: "", + Number: 0, + Comment: "@pr-reviewer's review-bot says:\n\n* \n\nThis PR satisfies all automated review criteria.\n\n/lgtm\n/approve\n\nThis PR does not require further manual action.\n\n**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**\n", + }, + }, + }, + { + name: "review approved", + fields: fields{ + l: newEntry(), + org: "", + repo: "", + num: 0, + user: "", + action: "", + dryRun: false, + BaseSHA: "", + }, + args: args{ + githubClient: newGHReviewClient(), + botReviewResults: []BotReviewResult{ + NewShouldNotMergeReviewResult("approved", "disapproved", "should not get merged at all reason"), + }, + }, + wantErr: false, + wantReviewComments: []*FakeComment{ + { + Org: "", + Repo: "", + Number: 0, + Comment: "@pr-reviewer's review-bot says:\n\n* approved\n\nThis PR satisfies all automated review criteria.\n\n/lgtm\n/approve\n\nHolding this PR because:\n* should not get merged at all reason\n\n/hold\n\n**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**\n", + }, + }, + }, + { + name: "review not approved", + fields: fields{ + l: newEntry(), + org: "", + repo: "", + num: 0, + user: "", + action: "", + dryRun: false, + BaseSHA: "", + }, + args: args{ + githubClient: newGHReviewClient(), + botReviewResults: []BotReviewResult{ + newReviewResultWithData( + "approved", + "disapproved", + map[string][]*diff.Hunk{ + "test": { + { + Body: []byte("nil"), + }, + }, + }, + "should not get merged at all reason", + ), + }, + }, + wantErr: false, + wantReviewComments: []*FakeComment{ + { + Org: "", + Repo: "", + Number: 0, + Comment: "@pr-reviewer's review-bot says:\n\n* disapproved\nFile: `test`\n```\nnil\n```\n\nThis PR does not satisfy at least one automated review criteria.\n\nHolding this PR because:\n* should not get merged at all reason\n\n/hold\n\n**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**\n", + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &Reviewer{ + l: tt.fields.l, + org: tt.fields.org, + repo: tt.fields.repo, + num: tt.fields.num, + user: tt.fields.user, + action: tt.fields.action, + dryRun: tt.fields.dryRun, + BaseSHA: tt.fields.BaseSHA, + } + if err := r.AttachReviewComments(tt.args.botReviewResults, &tt.args.githubClient); (err != nil) != tt.wantErr { + t.Errorf("AttachReviewComments() error = %v, wantErr %v", err, tt.wantErr) + } + if !reflect.DeepEqual(tt.args.githubClient.FakeComments, tt.wantReviewComments) { + t.Errorf("AttachReviewComments() reviewComments = %v, wantReviewComments %v", tt.args.githubClient.FakeComments, tt.wantReviewComments) + } + }) + } +} + +func newEntry() *logrus.Entry { + return logrus.NewEntry(logrus.StandardLogger()) +} + +type FakeComment struct { + Org string `json:"org,omitempty"` + Repo string `json:"repo,omitempty"` + Number int `json:"number,omitempty"` + Comment string `json:"comment,omitempty"` +} + +func (f FakeComment) String() string { + marshal, _ := json.Marshal(f) + return string(marshal) +} + +type FakeGHReviewClient struct { + FakeComments []*FakeComment +} + +func (f *FakeGHReviewClient) CreateComment(org, repo string, number int, comment string) error { + f.FakeComments = append(f.FakeComments, &FakeComment{ + Org: org, + Repo: repo, + Number: number, + Comment: comment, + }) + return nil +} + +func (f *FakeGHReviewClient) BotUser() (*github.UserData, error) { + return &github.UserData{ + Login: "pr-reviewer", + }, nil +} + +func newGHReviewClient() FakeGHReviewClient { + return FakeGHReviewClient{} +} From fe14714b9251eee2ff5697d1642e727a2c9ac7ea Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Wed, 13 Dec 2023 18:00:06 +0100 Subject: [PATCH 3/8] botreview, comments: create details section Move the details of the review into a Markdown details section, so that they do not overflow the comment for the review. Signed-off-by: Daniel Hiller --- external-plugins/botreview/review/BUILD.bazel | 7 +- external-plugins/botreview/review/result.go | 6 +- .../botreview/review/review_test.go | 171 ++++++++++++------ go.mod | 1 + go.sum | 1 + 5 files changed, 129 insertions(+), 57 deletions(-) diff --git a/external-plugins/botreview/review/BUILD.bazel b/external-plugins/botreview/review/BUILD.bazel index 44fc8a3e88..fb549ae78f 100644 --- a/external-plugins/botreview/review/BUILD.bazel +++ b/external-plugins/botreview/review/BUILD.bazel @@ -29,5 +29,10 @@ go_test( ], data = glob(["testdata/**"]), embed = [":go_default_library"], - deps = ["@com_github_sourcegraph_go_diff//diff:go_default_library"], + deps = [ + "@com_github_andreyvit_diff//:go_default_library", + "@com_github_sirupsen_logrus//:go_default_library", + "@com_github_sourcegraph_go_diff//diff:go_default_library", + "@io_k8s_test_infra//prow/github:go_default_library", + ], ) diff --git a/external-plugins/botreview/review/result.go b/external-plugins/botreview/review/result.go index 8f6fc4087a..0b5e5cd5a9 100644 --- a/external-plugins/botreview/review/result.go +++ b/external-plugins/botreview/review/result.go @@ -78,10 +78,12 @@ func (r *BasicReviewResult) String() string { } else { comment := r.disapproveComment for fileName, hunks := range r.notMatchingHunks { - comment += fmt.Sprintf("\nFile: `%s`", fileName) + comment += fmt.Sprintf("\n
") + comment += fmt.Sprintf("\n `%s`", fileName) for _, hunk := range hunks { - comment += fmt.Sprintf("\n```\n%s\n```", string(hunk.Body)) + comment += fmt.Sprintf("\n ```diff\n %s\n ```", string(hunk.Body)) } + comment += fmt.Sprintf("\n
") } return comment } diff --git a/external-plugins/botreview/review/review_test.go b/external-plugins/botreview/review/review_test.go index 4a312f2504..21502c1c49 100644 --- a/external-plugins/botreview/review/review_test.go +++ b/external-plugins/botreview/review/review_test.go @@ -20,6 +20,7 @@ package review import ( "encoding/json" + testdiff "github.com/andreyvit/diff" "github.com/sirupsen/logrus" "github.com/sourcegraph/go-diff/diff" "k8s.io/test-infra/prow/github" @@ -178,17 +179,31 @@ func TestGuessReviewTypes(t *testing.T) { } } -func TestReviewer_AttachReviewComments(t *testing.T) { - type fields struct { - l *logrus.Entry - org string - repo string - num int - user string - action github.PullRequestEventAction - dryRun bool - BaseSHA string +type fields struct { + l *logrus.Entry + org string + repo string + num int + user string + action github.PullRequestEventAction + dryRun bool + BaseSHA string +} + +func newFields() fields { + return fields{ + l: newEntry(), + org: "", + repo: "", + num: 0, + user: "", + action: "", + dryRun: false, + BaseSHA: "", } +} +func TestReviewer_AttachReviewComments(t *testing.T) { + type args struct { botReviewResults []BotReviewResult githubClient FakeGHReviewClient @@ -201,17 +216,8 @@ func TestReviewer_AttachReviewComments(t *testing.T) { wantReviewComments []*FakeComment }{ { - name: "basic comment", - fields: fields{ - l: newEntry(), - org: "", - repo: "", - num: 0, - user: "", - action: "", - dryRun: false, - BaseSHA: "", - }, + name: "basic comment", + fields: newFields(), args: args{ githubClient: newGHReviewClient(), botReviewResults: []BotReviewResult{}, @@ -227,17 +233,8 @@ func TestReviewer_AttachReviewComments(t *testing.T) { }, }, { - name: "review approved", - fields: fields{ - l: newEntry(), - org: "", - repo: "", - num: 0, - user: "", - action: "", - dryRun: false, - BaseSHA: "", - }, + name: "review approved", + fields: newFields(), args: args{ githubClient: newGHReviewClient(), botReviewResults: []BotReviewResult{ @@ -255,30 +252,15 @@ func TestReviewer_AttachReviewComments(t *testing.T) { }, }, { - name: "review not approved", - fields: fields{ - l: newEntry(), - org: "", - repo: "", - num: 0, - user: "", - action: "", - dryRun: false, - BaseSHA: "", - }, + name: "one review not approved", + fields: newFields(), args: args{ githubClient: newGHReviewClient(), botReviewResults: []BotReviewResult{ newReviewResultWithData( "approved", "disapproved", - map[string][]*diff.Hunk{ - "test": { - { - Body: []byte("nil"), - }, - }, - }, + map[string][]*diff.Hunk{"test": {{Body: []byte("nil")}}}, "should not get merged at all reason", ), }, @@ -286,10 +268,84 @@ func TestReviewer_AttachReviewComments(t *testing.T) { wantErr: false, wantReviewComments: []*FakeComment{ { - Org: "", - Repo: "", - Number: 0, - Comment: "@pr-reviewer's review-bot says:\n\n* disapproved\nFile: `test`\n```\nnil\n```\n\nThis PR does not satisfy at least one automated review criteria.\n\nHolding this PR because:\n* should not get merged at all reason\n\n/hold\n\n**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**\n", + Org: "", + Repo: "", + Number: 0, + Comment: `@pr-reviewer's review-bot says: + +* disapproved +
+ ` + "`test`" + ` + ` + "```diff" + ` + nil + ` + "```" + ` +
+ +This PR does not satisfy at least one automated review criteria. + +Holding this PR because: +* should not get merged at all reason + +/hold + +**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!** +`, + }, + }, + }, + { + name: "two reviews not approved", + fields: newFields(), + args: args{ + githubClient: newGHReviewClient(), + botReviewResults: []BotReviewResult{ + newReviewResultWithData( + "approved", + "can't approve moo", + map[string][]*diff.Hunk{"mehFile": {{Body: []byte("moo")}}}, + "should not get merged at all", + ), + newReviewResultWithData( + "approved", + "will not approve meh", + map[string][]*diff.Hunk{"mooFile": {{Body: []byte("meh")}}}, + "should not get merged in any case", + ), + }, + }, + wantErr: false, + wantReviewComments: []*FakeComment{ + { + Org: "", + Repo: "", + Number: 0, + Comment: `@pr-reviewer's review-bot says: + +* can't approve moo +
+ ` + "`mehFile`" + ` + ` + "```diff" + ` + moo + ` + "```" + ` +
+* will not approve meh +
+ ` + "`mooFile`" + ` + ` + "```diff" + ` + meh + ` + "```" + ` +
+ +This PR does not satisfy at least one automated review criteria. + +Holding this PR because: +* should not get merged at all +* should not get merged in any case + +/hold + +**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!** +`, }, }, }, @@ -311,6 +367,13 @@ func TestReviewer_AttachReviewComments(t *testing.T) { } if !reflect.DeepEqual(tt.args.githubClient.FakeComments, tt.wantReviewComments) { t.Errorf("AttachReviewComments() reviewComments = %v, wantReviewComments %v", tt.args.githubClient.FakeComments, tt.wantReviewComments) + for i, comment := range tt.wantReviewComments { + if len(tt.args.githubClient.FakeComments) <= i { + t.Errorf("no comment on %d found", i) + continue + } + t.Errorf("diff:\n%s", testdiff.LineDiff(tt.args.githubClient.FakeComments[i].Comment, comment.Comment)) + } } }) } diff --git a/go.mod b/go.mod index 89da577744..9807d8e122 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ require ( cloud.google.com/go/storage v1.35.1 github.com/Masterminds/semver v1.5.0 github.com/MetalBlueberry/go-plotly v0.4.0 + github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 github.com/avast/retry-go v3.0.0+incompatible github.com/bazelbuild/buildtools v0.0.0-20200922170545-10384511ce98 github.com/bndr/gojenkins v1.1.0 diff --git a/go.sum b/go.sum index 77ff80fea8..fadb12123b 100644 --- a/go.sum +++ b/go.sum @@ -669,6 +669,7 @@ github.com/alecthomas/template v0.0.0-20190718012654-fb15b899a751/go.mod h1:LOuy github.com/alecthomas/units v0.0.0-20151022065526-2efee857e7cf/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= github.com/alecthomas/units v0.0.0-20190717042225-c3de453c63f4/go.mod h1:ybxpYRFXyAe+OPACYpWeL0wqObRcbAqCMya13uyzqw0= github.com/alecthomas/units v0.0.0-20190924025748-f65c72e2690d/go.mod h1:rBZYJk541a8SKzHPHnH3zbiI+7dagKZ0cgpgrD7Fyho= +github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 h1:bvNMNQO63//z+xNgfBlViaCIJKLlCJ6/fmUseuG0wVQ= github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8= github.com/andybalholm/brotli v0.0.0-20190621154722-5f990b63d2d6/go.mod h1:+lx6/Aqd1kLJ1GQfkvOnaZ1WGmLpMpbprPuIOOZX30U= github.com/andybalholm/brotli v1.0.4/go.mod h1:fO7iG3H7G2nSZ7m0zPUDn85XEX2GTukHGRSepvi9Eig= From 93fa38cbeb26a1911c0f23bcdd39ec57edd042de Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Wed, 13 Dec 2023 18:15:17 +0100 Subject: [PATCH 4/8] botreview, comment: convert file list into md bullets Reduce list of files (in case of a very long comment) into a real bullet list. Signed-off-by: Daniel Hiller --- external-plugins/botreview/review/result.go | 5 +- .../botreview/review/result_test.go | 77 +++++++++++++++++++ 2 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 external-plugins/botreview/review/result_test.go diff --git a/external-plugins/botreview/review/result.go b/external-plugins/botreview/review/result.go index 0b5e5cd5a9..d4f7f253ea 100644 --- a/external-plugins/botreview/review/result.go +++ b/external-plugins/botreview/review/result.go @@ -113,10 +113,11 @@ func (r *BasicReviewResult) ShortString() string { return r.approveComment } else { comment := r.disapproveComment - comment += fmt.Sprintf("\nFiles:") + comment += fmt.Sprintf("\n
") for fileName := range r.notMatchingHunks { - comment += fmt.Sprintf("\n* `%s`", fileName) + comment += fmt.Sprintf("\n * `%s`", fileName) } + comment += fmt.Sprintf("\n
") return comment } } diff --git a/external-plugins/botreview/review/result_test.go b/external-plugins/botreview/review/result_test.go new file mode 100644 index 0000000000..a08d74167a --- /dev/null +++ b/external-plugins/botreview/review/result_test.go @@ -0,0 +1,77 @@ +/* + * This file is part of the KubeVirt project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * Copyright the KubeVirt Authors. + */ + +package review + +import ( + "github.com/sourcegraph/go-diff/diff" + "testing" +) + +func TestBasicReviewResult_ShortString(t *testing.T) { + type fields struct { + approveComment string + disapproveComment string + notMatchingHunks map[string][]*diff.Hunk + shouldNotMergeReason string + } + tests := []struct { + name string + fields fields + want string + }{ + { + name: "empty not matching hunks should only return the approve comment", + fields: fields{ + approveComment: "approved", + disapproveComment: "disapproved", + notMatchingHunks: nil, + shouldNotMergeReason: "", + }, + want: "approved", + }, + { + name: "short string should be a bullet list of files", + fields: fields{ + approveComment: "approved", + disapproveComment: "disapproved", + notMatchingHunks: map[string][]*diff.Hunk{ + "meh": {{Body: []byte("blah")}}, + }, + shouldNotMergeReason: "", + }, + want: `disapproved +
+ * ` + "`meh`" + ` +
`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &BasicReviewResult{ + approveComment: tt.fields.approveComment, + disapproveComment: tt.fields.disapproveComment, + notMatchingHunks: tt.fields.notMatchingHunks, + shouldNotMergeReason: tt.fields.shouldNotMergeReason, + } + if got := r.ShortString(); got != tt.want { + t.Errorf("ShortString() = %v, want %v", got, tt.want) + } + }) + } +} From 85729e1249f1f9d5e38d2be8e3c9011066af11d3 Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Thu, 14 Dec 2023 11:24:29 +0100 Subject: [PATCH 5/8] botreview, kubevirtci-bump: handle deleted files case If files are deleted inside the provider bump, this is still ok, so handle file deletions as regular changes. Signed-off-by: Daniel Hiller --- .../botreview/review/bump_kubevirtci.go | 11 +- .../botreview/review/bump_kubevirtci_test.go | 24 ++++ .../botreview/review/image_update.go | 5 +- .../botreview/review/prow_autobump.go | 7 +- .../bump-kci.patch00 | 7 ++ .../bump-kci.patch01 | 104 ++++++++++++++++++ .../bump-kci.patch02 | 62 +++++++++++ .../bump-kci.patch03 | 10 ++ .../bump-kci.patch04 | 7 ++ .../bump-kci.patch05 | 13 +++ 10 files changed, 237 insertions(+), 13 deletions(-) create mode 100644 external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch03 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch04 create mode 100644 external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch05 diff --git a/external-plugins/botreview/review/bump_kubevirtci.go b/external-plugins/botreview/review/bump_kubevirtci.go index dea2c03135..4d35cd78c6 100644 --- a/external-plugins/botreview/review/bump_kubevirtci.go +++ b/external-plugins/botreview/review/bump_kubevirtci.go @@ -28,10 +28,7 @@ import ( const ( bumpKubevirtCIApproveComment = `:thumbsup: This looks like a simple kubevirtci bump.` - bumpKubevirtCIDisapproveComment = `:thumbsdown: This doesn't look like a simple kubevirtci bump. - -I found suspicious hunks: -` + bumpKubevirtCIDisapproveComment = `:thumbsdown: This doesn't look like a simple kubevirtci bump.` ) var bumpKubevirtCIHackConfigDefaultMatcher *regexp.Regexp @@ -56,6 +53,12 @@ func (t *BumpKubevirtCI) IsRelevant() bool { func (t *BumpKubevirtCI) AddIfRelevant(fileDiff *diff.FileDiff) { fileName := strings.TrimPrefix(fileDiff.NewName, "b/") + // handle deleted files + wasDeleted := fileName == "/dev/null" + if wasDeleted { + fileName = strings.TrimPrefix(fileDiff.OrigName, "a/") + } + if fileName == "cluster-up-sha.txt" || fileName == "hack/config-default.sh" || strings.HasPrefix(fileName, "cluster-up/") { diff --git a/external-plugins/botreview/review/bump_kubevirtci_test.go b/external-plugins/botreview/review/bump_kubevirtci_test.go index 104f39a603..63ae06f47d 100644 --- a/external-plugins/botreview/review/bump_kubevirtci_test.go +++ b/external-plugins/botreview/review/bump_kubevirtci_test.go @@ -20,6 +20,7 @@ package review import ( + "fmt" "github.com/sourcegraph/go-diff/diff" "os" "path/filepath" @@ -46,6 +47,12 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) { "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch14", "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch15", "testdata/kubevirt/fix-containerdisks-migration/fix-containerdisks-migrations.patch16", + "testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00", + "testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01", + "testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02", + "testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch03", + "testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch04", + "testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch05", } entries, err := os.ReadDir("testdata/kubevirtci-bump") if err != nil { @@ -65,6 +72,9 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) { if err != nil { t1.Errorf("failed to read diff: %v", err) } + if bumpFileDiffs == nil { + panic(fmt.Sprintf("file diff %q empty", diffFile)) + } diffFilePathsToDiffs[diffFile] = bumpFileDiffs } type fields struct { @@ -145,6 +155,20 @@ func TestBumpKubevirtCI_Review(t1 *testing.T) { "pkg/container-disk/container-disk.go": nil, }, ""), }, + { + name: "kubevirtci-bump with deleted files", + fields: fields{ + relevantFileDiffs: []*diff.FileDiff{ + diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00"], + diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01"], + diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02"], + diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch03"], + diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch04"], + diffFilePathsToDiffs["testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch05"], + }, + }, + want: newReviewResultWithData(bumpKubevirtCIApproveComment, bumpKubevirtCIDisapproveComment, nil, ""), + }, } for _, tt := range tests { t1.Run(tt.name, func(t1 *testing.T) { diff --git a/external-plugins/botreview/review/image_update.go b/external-plugins/botreview/review/image_update.go index 2ea3aae78a..964aab0587 100644 --- a/external-plugins/botreview/review/image_update.go +++ b/external-plugins/botreview/review/image_update.go @@ -28,10 +28,7 @@ import ( const ( prowJobImageUpdateApproveComment = `:thumbsup: This looks like a simple prow job image bump.` - prowJobImageUpdateDisapproveComment = `:thumbsdown: This doesn't look like a simple prow job image bump. - -I found suspicious hunks: -` + prowJobImageUpdateDisapproveComment = `:thumbsdown: This doesn't look like a simple prow job image bump.` ) var ( diff --git a/external-plugins/botreview/review/prow_autobump.go b/external-plugins/botreview/review/prow_autobump.go index 96ff0b60a4..4e07f03655 100644 --- a/external-plugins/botreview/review/prow_autobump.go +++ b/external-plugins/botreview/review/prow_autobump.go @@ -27,11 +27,8 @@ import ( ) const ( - prowAutobumpApproveComment = `:thumbsup: This looks like a simple prow autobump.` - prowAutobumpDisapproveComment = `:thumbsdown: This doesn't look like a simple prow autobump. - -I found suspicious hunks: -` + prowAutobumpApproveComment = `:thumbsup: This looks like a simple prow autobump.` + prowAutobumpDisapproveComment = `:thumbsdown: This doesn't look like a simple prow autobump.` prowAutoBumpShouldNotMergeReason = "prow update should be merged at a point in time where it doesn't interfere with normal CI usage" ) diff --git a/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00 b/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00 new file mode 100644 index 0000000000..fe72b33619 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch00 @@ -0,0 +1,7 @@ +diff --git a/cluster-up-sha.txt b/cluster-up-sha.txt +index 88f70e89fa..3073b8af4b 100644 +--- a/cluster-up-sha.txt ++++ b/cluster-up-sha.txt +@@ -1 +1 @@ +-a6b18056244c8204726bc5e4e9c49e16a496db6a ++896ed368bdb91569a941304c79bbb84b80f181c1 diff --git a/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01 b/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01 new file mode 100644 index 0000000000..743e662180 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch01 @@ -0,0 +1,104 @@ +diff --git a/cluster-up/cluster/local/README.md b/cluster-up/cluster/local/README.md +deleted file mode 100644 +index 6b8972549a..0000000000 +--- a/cluster-up/cluster/local/README.md ++++ /dev/null +@@ -1,98 +0,0 @@ +-# Local Kubernetes Provider +- +-This provider allows developing against bleeding-edge Kubernetes code. The +-k8s sources will be compiled and a single-node cluster will be started. +- +-## Environment preparation +- +-Since the `local` provider deploys the cluster on the host and not inside +-virtual machines, you may need to adjust some settings on the node. +- +-Specifically, you may need to make sure that your firewall of choice doesn't +-block connectivity between cluster IP and service pods. If you experience +-connectivity issues, consider tweaking or disabling your firewall. Also, make +-sure forwarding is enabled on the host: +- +-```bash +-$ systemctl disable firewalld --now +-$ iptables -P FORWARD ACCEPT +-$ sysctl net.ipv4.conf.all.forwarding=1 +-``` +- +-## Bringing the cluster up +- +-First get the k8s sources: +- +-```bash +-go get -u -d k8s.io/kubernetes +-``` +- +-Then compile and start the cluster: +- +-```bash +-export KUBEVIRT_PROVIDER=local +-make cluster-up +-``` +- +-The cluster can be accessed as usual: +- +-```bash +-$ cluster/kubectl.sh get nodes +-NAME STATUS ROLES AGE VERSION +-kubdev Ready 5m20s v1.12.0-beta.2 +-``` +- +-Note: you may need to cherry-pick +-[acdb1b0e9855ab671f2972f10605d20cad26284b](https://github.com/kubernetes/kubernetes/commit/acdb1b0e9855ab671f2972f10605d20cad26284b) +-if it's not present in your kubernetes tree yet. +- +-## CNI +- +-By default, local provider deploys cluster with no CNI support. To make CNI +-work, you should set the following variables before spinning up cluster: +- +-```bash +-$ export NET_PLUGIN=cni +-$ export CNI_CONF_DIR=/etc/cni/net.d/ +-$ export CNI_BIN_DIR=/opt/cni/bin/ +-``` +- +-Depending on your CNI of choice (for example, Flannel), you may also need to +-add the following arguments to controller-manager inside +-`hack/local-cluster-up.sh`: +- +-```bash +-$ git diff +-diff --git a/hack/local-up-cluster.sh b/hack/local-up-cluster.sh +-index bcf988b..9911eed 100755 +---- a/hack/local-up-cluster.sh +-+++ b/hack/local-up-cluster.sh +-@@ -639,6 +639,8 @@ function start_controller_manager { +- --use-service-account-credentials \ +- --controllers="${KUBE_CONTROLLERS}" \ +- --leader-elect=false \ +- --cert-dir="$CERT_DIR" \ +-+ --allocate-node-cidrs=true --cluster-cidr=10.244.0.0/16 \ +- --master="https://${API_HOST}:${API_SECURE_PORT}" >"${CTLRMGR_LOG}" 2>&1 & +- CTLRMGR_PID=$! +- } +-``` +- +-Also, you will need to install [reference CNI plugins](https://github.com/containernetworking/plugins): +- +-```bash +-$ go get -u -d github.com/containernetworking/plugins/ +-$ cd $GOPATH/src/github.com/containernetworking/plugins/ +-$ ./build.sh +-$ sudo mkdir -p /opt/cni/bin/ +-$ sudo cp bin/* /opt/cni/bin/ +-``` +- +-In some cases (for example, Multus), your CNI plugin may also require presence +-of `/etc/kubernetes/kubelet.conf` file. In this case, you should create a +-symlink pointing to the right location: +- +-```bash +-$ sudo mkdir /etc/kubernetes +-$ sudo ln -s $GOPATH/src/kubevirt.io/kubevirt/cluster/local/certs/kubelet.kubeconfig /etc/kubernetes/kubelet.conf +-``` diff --git a/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02 b/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02 new file mode 100644 index 0000000000..5f78a309b3 --- /dev/null +++ b/external-plugins/botreview/review/testdata/kubevirt/ci-bump-remove-provider-local/bump-kci.patch02 @@ -0,0 +1,62 @@ +diff --git a/cluster-up/cluster/local/provider.sh b/cluster-up/cluster/local/provider.sh +deleted file mode 100644 +index af10d52f5b..0000000000 +--- a/cluster-up/cluster/local/provider.sh ++++ /dev/null +@@ -1,56 +0,0 @@ +-#!/usr/bin/env bash +- +-function _cert_dir() { +- echo $GOPATH/src/kubevirt.io/kubevirt/cluster/local/certs +-} +- +-function _main_ip() { +- ip -o -4 a | tr -s ' ' | cut -d' ' -f 2,4 | +- grep -v -e '^lo[0-9:]*' | head -1 | +- cut -d' ' -f 2 | cut -d'/' -f1 +-} +- +-function up() { +- # Make sure that local config is correct +- prepare_config +- +- go get -d k8s.io/kubernetes +- +- export API_HOST_IP=$(_main_ip) +- export KUBELET_HOST=$(_main_ip) +- export HOSTNAME_OVERRIDE=kubdev +- export ALLOW_PRIVILEGED=1 +- export ALLOW_SECURITY_CONTEXT=1 +- export KUBE_DNS_DOMAIN="cluster.local" +- export KUBE_DNS_SERVER_IP="10.0.0.10" +- export KUBE_ENABLE_CLUSTER_DNS=true +- export CERT_DIR=$(_cert_dir) +- ( +- cd $GOPATH/src/k8s.io/kubernetes +- ./hack/local-up-cluster.sh +- ) +-} +- +-function prepare_config() { +- PROVIDER_CONFIG_FILE_PATH="${BASE_PATH}/$KUBEVIRT_PROVIDER/config-provider-$KUBEVIRT_PROVIDER.sh" +- cat > "$PROVIDER_CONFIG_FILE_PATH" < Date: Fri, 15 Dec 2023 14:18:31 +0100 Subject: [PATCH 6/8] fix, bazel, botreview: update build file Signed-off-by: Daniel Hiller --- external-plugins/botreview/review/BUILD.bazel | 1 + 1 file changed, 1 insertion(+) diff --git a/external-plugins/botreview/review/BUILD.bazel b/external-plugins/botreview/review/BUILD.bazel index fb549ae78f..b9e8fdfded 100644 --- a/external-plugins/botreview/review/BUILD.bazel +++ b/external-plugins/botreview/review/BUILD.bazel @@ -25,6 +25,7 @@ go_test( "bump_kubevirtci_test.go", "image_update_test.go", "prow_autobump_test.go", + "result_test.go", "review_test.go", ], data = glob(["testdata/**"]), From 3b70cf45a4696490b4c37cd935e6fc6055c3224b Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Wed, 20 Dec 2023 19:42:04 +0100 Subject: [PATCH 7/8] fix, botreview: fix comment formatting Signed-off-by: Daniel Hiller --- external-plugins/botreview/review/result.go | 8 +- external-plugins/botreview/review/review.go | 2 +- .../botreview/review/review_test.go | 100 +++++++++++++----- 3 files changed, 76 insertions(+), 34 deletions(-) diff --git a/external-plugins/botreview/review/result.go b/external-plugins/botreview/review/result.go index d4f7f253ea..a93cada3d2 100644 --- a/external-plugins/botreview/review/result.go +++ b/external-plugins/botreview/review/result.go @@ -78,12 +78,12 @@ func (r *BasicReviewResult) String() string { } else { comment := r.disapproveComment for fileName, hunks := range r.notMatchingHunks { - comment += fmt.Sprintf("\n
") - comment += fmt.Sprintf("\n `%s`", fileName) + comment += fmt.Sprintf("\n\n
") + comment += fmt.Sprintf("\n\n_%s_", fileName) for _, hunk := range hunks { - comment += fmt.Sprintf("\n ```diff\n %s\n ```", string(hunk.Body)) + comment += fmt.Sprintf("\n\n~~~diff\n%s\n~~~", string(hunk.Body)) } - comment += fmt.Sprintf("\n
") + comment += fmt.Sprintf("\n\n
\n") } return comment } diff --git a/external-plugins/botreview/review/review.go b/external-plugins/botreview/review/review.go index 81e55c7a43..826d34a103 100644 --- a/external-plugins/botreview/review/review.go +++ b/external-plugins/botreview/review/review.go @@ -187,7 +187,7 @@ func (r *Reviewer) AttachReviewComments(botReviewResults []BotReviewResult, gith botReviewComment := fmt.Sprintf( botReviewCommentPattern, botUser.Login, - newBulletList(botReviewComments), + strings.Join(botReviewComments, "\n"), approveLabels, holdComment, ) diff --git a/external-plugins/botreview/review/review_test.go b/external-plugins/botreview/review/review_test.go index 21502c1c49..fd17f9f8aa 100644 --- a/external-plugins/botreview/review/review_test.go +++ b/external-plugins/botreview/review/review_test.go @@ -225,10 +225,22 @@ func TestReviewer_AttachReviewComments(t *testing.T) { wantErr: false, wantReviewComments: []*FakeComment{ { - Org: "", - Repo: "", - Number: 0, - Comment: "@pr-reviewer's review-bot says:\n\n* \n\nThis PR satisfies all automated review criteria.\n\n/lgtm\n/approve\n\nThis PR does not require further manual action.\n\n**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**\n", + Org: "", + Repo: "", + Number: 0, + Comment: `@pr-reviewer's review-bot says: + + + +This PR satisfies all automated review criteria. + +/lgtm +/approve + +This PR does not require further manual action. + +**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!** +`, }, }, }, @@ -244,10 +256,25 @@ func TestReviewer_AttachReviewComments(t *testing.T) { wantErr: false, wantReviewComments: []*FakeComment{ { - Org: "", - Repo: "", - Number: 0, - Comment: "@pr-reviewer's review-bot says:\n\n* approved\n\nThis PR satisfies all automated review criteria.\n\n/lgtm\n/approve\n\nHolding this PR because:\n* should not get merged at all reason\n\n/hold\n\n**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!**\n", + Org: "", + Repo: "", + Number: 0, + Comment: `@pr-reviewer's review-bot says: + +approved + +This PR satisfies all automated review criteria. + +/lgtm +/approve + +Holding this PR because: +* should not get merged at all reason + +/hold + +**Note: botreview (kubevirt/project-infra#3100) is a Work In Progress!** +`, }, }, }, @@ -273,13 +300,18 @@ func TestReviewer_AttachReviewComments(t *testing.T) { Number: 0, Comment: `@pr-reviewer's review-bot says: -* disapproved -
- ` + "`test`" + ` - ` + "```diff" + ` - nil - ` + "```" + ` -
+disapproved + +
+ +_test_ + +~~~diff +nil +~~~ + +
+ This PR does not satisfy at least one automated review criteria. @@ -321,20 +353,30 @@ Holding this PR because: Number: 0, Comment: `@pr-reviewer's review-bot says: -* can't approve moo -
- ` + "`mehFile`" + ` - ` + "```diff" + ` - moo - ` + "```" + ` -
-* will not approve meh -
- ` + "`mooFile`" + ` - ` + "```diff" + ` - meh - ` + "```" + ` -
+can't approve moo + +
+ +_mehFile_ + +~~~diff +moo +~~~ + +
+ +will not approve meh + +
+ +_mooFile_ + +~~~diff +meh +~~~ + +
+ This PR does not satisfy at least one automated review criteria. From 575233d0601dc1430aa7cb9cad841ba70c720b17 Mon Sep 17 00:00:00 2001 From: Daniel Hiller Date: Thu, 21 Dec 2023 09:24:55 +0100 Subject: [PATCH 8/8] fix, botreview, image-update: fix regex Signed-off-by: Daniel Hiller --- .../botreview/review/image_update.go | 8 +- .../botreview/review/image_update_test.go | 102 ++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/external-plugins/botreview/review/image_update.go b/external-plugins/botreview/review/image_update.go index 964aab0587..6dbf388dcb 100644 --- a/external-plugins/botreview/review/image_update.go +++ b/external-plugins/botreview/review/image_update.go @@ -33,7 +33,7 @@ const ( var ( prowJobImageUpdateLineMatcher = regexp.MustCompile(`(?m)^\+\s+- image: \S+$`) - prowJobImageUpdateHunkBodyMatcher = regexp.MustCompile(`(?m)^(-\s+- image: \S+$\n^\+\s+- image: \S+|-\s+image: \S+$\n^\+\s+image: \S+)$`) + prowJobImageUpdateHunkBodyMatcher = regexp.MustCompile(`(?m)^(-\s+- image:\s+\S+$\n^\+\s+- image:\s+\S+|-\s+image:\s+\S+$\n^\+\s+image:\s+\S+)$`) prowJobReleaseBranchFileNameMatcher = regexp.MustCompile(`.*/[\w-]+-[0-9-.]+\.yaml`) ) @@ -83,7 +83,7 @@ func (t *ProwJobImageUpdate) Review() BotReviewResult { for _, fileDiff := range t.relevantFileDiffs { fileName := strings.TrimPrefix(fileDiff.NewName, "b/") for _, hunk := range fileDiff.Hunks { - if !prowJobImageUpdateHunkBodyMatcher.Match(hunk.Body) { + if !t.hunkMatches(hunk) { result.AddReviewFailure(fileName, hunk) } } @@ -92,6 +92,10 @@ func (t *ProwJobImageUpdate) Review() BotReviewResult { return result } +func (t *ProwJobImageUpdate) hunkMatches(hunk *diff.Hunk) bool { + return prowJobImageUpdateHunkBodyMatcher.Match(hunk.Body) +} + func (t *ProwJobImageUpdate) String() string { return fmt.Sprintf("relevantFileDiffs: %v", t.relevantFileDiffs) } diff --git a/external-plugins/botreview/review/image_update_test.go b/external-plugins/botreview/review/image_update_test.go index cd6c7abe44..59c242f684 100644 --- a/external-plugins/botreview/review/image_update_test.go +++ b/external-plugins/botreview/review/image_update_test.go @@ -161,3 +161,105 @@ func TestProwJobImageUpdate_AddIfRelevant(t1 *testing.T) { }) } } + +func TestProwJobImageUpdate_hunkMatches(t1 *testing.T) { + type fields struct { + relevantFileDiffs []*diff.FileDiff + notMatchingHunks []*diff.Hunk + } + type args struct { + hunk *diff.Hunk + } + tests := []struct { + name string + fields fields + args args + want bool + }{ + { + name: "image update 1", + fields: fields{}, + args: args{ + hunk: &diff.Hunk{ + Body: []byte(` nodeSelector: + type: bare-metal-external + containers: +- - image: quay.io/kubevirtci/bootstrap:v20231115-51a244f ++ - image: quay.io/kubevirtci/bootstrap:v20231219-bf5e580 + command: + - "/usr/local/bin/runner.sh" + - "/bin/sh" +`), + }, + }, + want: true, + }, + { + name: "image update 2", + fields: fields{}, + args: args{ + hunk: &diff.Hunk{ + Body: []byte(` cluster: prow-workloads + spec: + containers: +- - image: quay.io/kubevirtci/bootstrap:v20201119-a5880e0 ++ - image: quay.io/kubevirtci/bootstrap:v20220110-c066ff5 + command: + - "/usr/local/bin/runner.sh" + - "/bin/bash" +`), + }, + }, + want: true, + }, + { + name: "image update 3", + fields: fields{}, + args: args{ + hunk: &diff.Hunk{ + Body: []byte(` nodeSelector: + type: bare-metal-external + containers: +- - image: quay.io/kubevirtci/kubevirt-infra-bootstrap:v20201201-08dc4a9 ++ - image: quay.io/kubevirtci/kubevirt-infra-bootstrap:v20210419-444033d + securityContext: + privileged: true + resources: +`), + }, + }, + want: true, + }, + { + name: "mixed update 1", + fields: fields{}, + args: args{ + hunk: &diff.Hunk{ + Body: []byte(` - automation/test.sh + env: + - name: TARGET +- value: k8s-1.21-sig-network +- image: quay.io/kubevirtci/bootstrap:v20220602-ac34bf7 ++ value: k8s-1.21-sig-storage ++ image: quay.io/kubevirtci/bootstrap:v20999999-eeff777 + name: "" + resources: + requests: +`), + }, + }, + want: false, + }, + } + for _, tt := range tests { + t1.Run(tt.name, func(t1 *testing.T) { + t := &ProwJobImageUpdate{ + relevantFileDiffs: tt.fields.relevantFileDiffs, + notMatchingHunks: tt.fields.notMatchingHunks, + } + if got := t.hunkMatches(tt.args.hunk); got != tt.want { + t1.Errorf("hunkMatches() = %v, want %v", got, tt.want) + } + }) + } +}