Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure sandbox toleration #535

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Ensure sandbox toleration #535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55ecc3f
Ensure sandbox toleration
rajivnathan Feb 26, 2024
a0ee25a
Prevent duplicate tolerations
rajivnathan Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions pkg/webhook/mutatingwebhook/vm_mutate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@

var vmLogger = logf.Log.WithName("virtual_machines_mutating_webhook")

var sandboxToleration = map[string]interface{}{
"effect": "NoSchedule",
"key": "sandbox-cnv",
"operator": "Exists",
}

func HandleMutateVirtualMachines(w http.ResponseWriter, r *http.Request) {
handleMutate(vmLogger, w, r, vmMutator)
}
Expand Down Expand Up @@ -48,6 +54,9 @@
// ensure limits are set in a best effort approach, if the limits are not set for any reason the request will still be allowed
vmPatchItems = ensureLimits(unstructuredRequestObj, vmPatchItems)

// ensure tolerations are set so vm can be scheduled to the metal node
vmPatchItems = ensureTolerations(unstructuredRequestObj, vmPatchItems)

// ensure cloud-init config is set, if the cloud-init config cannot be set for any reason the request will be blocked
var cloudInitConfigErr error
vmPatchItems, cloudInitConfigErr = ensureVolumeConfig(unstructuredRequestObj, vmPatchItems)
Expand Down Expand Up @@ -247,6 +256,27 @@
return patchItems
}

// ensureTolerations ensures tolerations are set on the VirtualMachine
func ensureTolerations(unstructuredRequestObj *unstructured.Unstructured, patchItems []map[string]interface{}) []map[string]interface{} {
// get existing tolerations, if any
tolerations, _, err := unstructured.NestedSlice(unstructuredRequestObj.Object, "spec", "template", "spec", "tolerations")
if err != nil {
vmLogger.Error(err, "unable to get tolerations from VirtualMachine", "VirtualMachine", unstructuredRequestObj)
return patchItems
}

Check warning on line 266 in pkg/webhook/mutatingwebhook/vm_mutate.go

View check run for this annotation

Codecov / codecov/patch

pkg/webhook/mutatingwebhook/vm_mutate.go#L264-L266 

Added lines #L264 - L266 were not covered by tests

for _, toleration := range tolerations {
tol, ok := toleration.(map[string]interface{})
if ok && tol["key"] == "sandbox-cnv" {
return patchItems // the toleration already exists
}
}
tolerations = append(tolerations, sandboxToleration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is already a sandbox toleration. We will have two copies of the same toleration with the same key:

{
	"effect":   "NoSchedule",
	"key":      "sandbox-cnv",
	"operator": "Exists",
},
{
	"effect":   "NoSchedule",
	"key":      "sandbox-cnv",
	"operator": "Exists",
}

right? Or I'm reading it wrong?

Copy link
Contributor Author

@rajivnathan rajivnathan Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I thought about adding a check for that case but

  1. the webhook is on create, so the toleration needs to be in the original VM for that to happen which is pretty unlikely
  2. it works correctly even if it adds a duplicate (I tested it)

I mostly ignored it because of 1...but now that I think about it again it's possible that a user copies the VM yaml for a VM that was created and has the toleration added by the webhook and later creates another VM manually using that yaml. In that case it'd add the toleration again which is ugly. So I'll add a check and avoid adding a toleration if there's an existing toleration with the sandbox-cnv key.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... and if in the future we start invoking the webhook on updates too (not just on create) then every update on the VM resource would result in another duplicate of the toleration.

Copy link
Contributor Author

@rajivnathan rajivnathan Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled duplicate check in a0ee25a . Thanks

patchItems = append(patchItems, addTolerations(tolerations))

return patchItems
}

func defaultUserData(sshKeys []string) string {
authorizedKeys := fmt.Sprintf("ssh_authorized_keys: [%s]\n", sshKeys)
return strings.Join(
Expand All @@ -273,3 +303,11 @@
"value": resources,
}
}

func addTolerations(tolerations []interface{}) map[string]interface{} {
return map[string]interface{}{
"op": "add",
"path": "/spec/template/spec/tolerations",
"value": tolerations,
}
}
70 changes: 68 additions & 2 deletions pkg/webhook/mutatingwebhook/vm_mutate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,19 @@ func TestVMMutator(t *testing.T) {
// given
admReview := admissionReview(t, vmRawAdmissionReviewJSONTemplate, setVolumes(rootDiskVolume(), cloudInitVolume(cloudInitNoCloud, userDataWithoutSSHKey)))
expectedVolumes := cloudInitVolume(cloudInitNoCloud, userDataWithSSHKey) // expect SSH key will be added to userData
expectedVolumesPatch := []map[string]interface{}{volumesPatch(expectedVolumes)}
expectedVolumesPatch := volumesPatch(expectedVolumes)
expectedTolerationsPatch := addTolerations([]interface{}{sandboxToleration})

// when
actualResponse := vmMutator(admReview)

// then
assert.Equal(t, expectedVMMutateRespSuccess(t, expectedVolumesPatch...), *actualResponse)
expectedPatches := []map[string]interface{}{expectedTolerationsPatch, expectedVolumesPatch}
actualPatchContent := actualResponse.Patch
expectedPatchContent, err := json.Marshal(expectedPatches)
require.NoError(t, err)
require.Equal(t, string(expectedPatchContent), string(actualPatchContent))
assert.Equal(t, expectedVMMutateRespSuccess(t, expectedTolerationsPatch, expectedVolumesPatch), *actualResponse)
})

t.Run("fail", func(t *testing.T) {
Expand Down Expand Up @@ -395,6 +401,59 @@ func TestAddSSHKeyToUserData(t *testing.T) {
}
}

func TestEnsureTolerations(t *testing.T) {

t.Run("success", func(t *testing.T) {
t.Run("no existing tolerations", func(t *testing.T) {
// given
vmAdmReviewRequestObj := vmAdmReviewRequestObject(t)
// expect only sandbox toleration
expectedPatchItems := []map[string]interface{}{addTolerations([]interface{}{sandboxToleration})}

// when
actualPatchItems := ensureTolerations(vmAdmReviewRequestObj, []map[string]interface{}{})

// then
assertPatchesEqual(t, expectedPatchItems, actualPatchItems)
})

t.Run("some existing tolerations", func(t *testing.T) {
// given
existingToleration := map[string]interface{}{
"effect": "NoSchedule",
"key": "another",
"operator": "Exists",
}
// expect existing toleration and sandbox toleration
vmAdmReviewRequestObj := vmAdmReviewRequestObject(t, setTolerations(existingToleration))
expectedPatchItems := []map[string]interface{}{addTolerations([]interface{}{existingToleration, sandboxToleration})} // patch should include existing toleration

// when
actualPatchItems := ensureTolerations(vmAdmReviewRequestObj, []map[string]interface{}{})

// then
assertPatchesEqual(t, expectedPatchItems, actualPatchItems)
})

t.Run("existing sandbox-cnv toleration", func(t *testing.T) {
// given
anotherToleration := map[string]interface{}{
"effect": "NoSchedule",
"key": "another",
"operator": "Exists",
}
// expect existing toleration and sandbox toleration
vmAdmReviewRequestObj := vmAdmReviewRequestObject(t, setTolerations(anotherToleration, sandboxToleration))

// when
actualPatchItems := ensureTolerations(vmAdmReviewRequestObj, []map[string]interface{}{})

// then
assert.Empty(t, actualPatchItems) // sandbox toleration exists so patch not needed
})
})
}

type admissionReviewOption func(t *testing.T, unstructuredAdmReview *unstructured.Unstructured)

func setDomainResourcesRequests(requests map[string]string) admissionReviewOption {
Expand Down Expand Up @@ -425,6 +484,13 @@ func setVolumes(volumes ...interface{}) admissionReviewOption {
}
}

func setTolerations(tolerations ...interface{}) admissionReviewOption {
return func(t *testing.T, unstructuredAdmReview *unstructured.Unstructured) {
err := unstructured.SetNestedSlice(unstructuredAdmReview.Object, tolerations, "request", "object", "spec", "template", "spec", "tolerations")
require.NoError(t, err)
}
}

func volumesPatch(expectedCloudInitDiskVolume map[string]interface{}) map[string]interface{} {
volumes := []interface{}{
rootDiskVolume(),
Expand Down
Loading