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.

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

OCPBUGS-28647: tuned: operand: add support for deferred updates #1019

Merged
merged 19 commits into from
Jul 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 13 additions & 10 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ pkg/generated: $(API_TYPES)
$(GOBINDATA_BIN):
$(GO) build -o $(GOBINDATA_BIN) ./vendor/github.com/kevinburke/go-bindata/go-bindata

test-e2e:
for d in core basic reboots reboots/sno; do \
test-e2e: $(BINDATA)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why we need to depend on $(BINDATA) for e2e tests, could you please explain?

Copy link
Contributor Author

@ffromani ffromani Jun 19, 2024

Choose a reason for hiding this comment

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

it's because the deferred tests import pkg/manifests

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for that. it causes me issues as well

for d in core basic reboots reboots/sno deferred; do \
KUBERNETES_CONFIG="$(KUBECONFIG)" $(GO) test -v -timeout 40m ./test/e2e/$$d -ginkgo.v -ginkgo.no-color -ginkgo.fail-fast || exit; \
done

Expand Down Expand Up @@ -218,7 +218,7 @@ cluster-deploy-pao:
pao-functests: cluster-label-worker-cnf pao-functests-only

.PHONY: pao-functests-only
pao-functests-only:
pao-functests-only: $(BINDATA)
@echo "Cluster Version"
hack/show-cluster-version.sh
hack/run-test.sh -t "test/e2e/performanceprofile/functests/0_config test/e2e/performanceprofile/functests/1_performance test/e2e/performanceprofile/functests/6_mustgather_testing test/e2e/performanceprofile/functests/10_performance_ppc" -p "-v -r --fail-fast --flake-attempts=2 --junit-report=report.xml" -m "Running Functional Tests"
Expand All @@ -227,7 +227,7 @@ pao-functests-only:
pao-functests-updating-profile: cluster-label-worker-cnf pao-functests-update-only

.PHONY: pao-functests-update-only
pao-functests-update-only:
pao-functests-update-only: $(BINDATA)
@echo "Cluster Version"
hack/show-cluster-version.sh
hack/run-test.sh -t "test/e2e/performanceprofile/functests/0_config test/e2e/performanceprofile/functests/2_performance_update test/e2e/performanceprofile/functests/3_performance_status test/e2e/performanceprofile/functests/7_performance_kubelet_node test/e2e/performanceprofile/functests/9_reboot" -p "-v -r --fail-fast --flake-attempts=2 --timeout=5h --junit-report=report.xml" -m "Running Functional Tests"
Expand All @@ -236,25 +236,25 @@ pao-functests-update-only:
pao-functests-performance-workloadhints: cluster-label-worker-cnf pao-functests-performance-workloadhints-only

.PHONY: pao-functests-performance-workloadhints-only
pao-functests-performance-workloadhints-only:
pao-functests-performance-workloadhints-only: $(BINDATA)
@echo "Cluster Version"
hack/show-cluster-version.sh
hack/run-test.sh -t "test/e2e/performanceprofile/functests/0_config test/e2e/performanceprofile/functests/8_performance_workloadhints" -p "-v -r --fail-fast --flake-attempts=2 --timeout=5h --junit-report=report.xml" -m "Running Functional WorkloadHints Tests"

.PHONY: pao-functests-latency-testing
pao-functests-latency-testing: dist-latency-tests
pao-functests-latency-testing: dist-latency-tests $(BINDATA)
@echo "Cluster Version"
hack/show-cluster-version.sh
hack/run-test.sh -t "./test/e2e/performanceprofile/functests/0_config ./test/e2e/performanceprofile/functests/5_latency_testing" -p "-v -r --fail-fast --flake-attempts=2 --timeout=5h --junit-report=report.xml" -m "Running Functionalconfiguration latency Tests"

.PHONY: pao-functests-mixedcpus
pao-functests-mixedcpus:
pao-functests-mixedcpus: $(BINDATA)
@echo "Cluster Version"
hack/show-cluster-version.sh
hack/run-test.sh -t "./test/e2e/performanceprofile/functests/0_config ./test/e2e/performanceprofile/functests/11_mixedcpus" -p "-v -r --fail-fast --flake-attempts=2 --junit-report=report.xml" -m "Running MixedCPUs Tests"

.PHONY: pao-functests-hypershift
pao-functests-hypershift:
pao-functests-hypershift: $(BINDATA)
@echo "Cluster Version"
hack/show-cluster-version.sh
hack/run-test.sh -t "./test/e2e/performanceprofile/functests/0_config" -p "-vv -r --fail-fast --flake-attempts=2 --junit-report=report.xml" -m "Running Functional Tests over Hypershift"
Expand Down Expand Up @@ -293,13 +293,16 @@ gather-sysinfo-tests: build-gather-sysinfo
render-sync: build
hack/render-sync.sh

build-e2e-%:
@hack/build-test-bin.sh $(shell echo $@ | sed -e 's/^build-e2e-//' )

pao-build-e2e-%:
@hack/build-test-bin.sh $(shell echo $@ | sed -e 's/^pao-build-e2e-//' )
@hack/build-pao-test-bin.sh $(shell echo $@ | sed -e 's/^pao-build-e2e-//' )

.PHONY: pao-build-e2e
pao-build-e2e:
@for suite in $(PAO_E2E_SUITES); do \
hack/build-test-bin.sh $$suite; \
hack/build-pao-test-bin.sh $$suite; \
done

.PHONY: pao-clean-e2e
Expand Down
30 changes: 30 additions & 0 deletions hack/build-pao-test-bin.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#!/bin/bash

set -e

PREFIX="pao-build-e2e-"
SUITEPATH="./test/e2e/performanceprofile/functests"
TARGET=$1

if [ -z "$TARGET" ]; then
echo "usage: $0 suite"
echo "example: $0 1_performance"
exit 1
fi

OUTDIR="${2:-_output}"

if [ ! -d "$SUITEPATH/$TARGET" ]; then
echo "unknown suite: $TARGET"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I'd argue that multi-line output provides better readability when using a here document, but feel free to keep this if you disagree. Similarly elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, but better to do this in a separate PR

echo -e "must be one of:\n$( ls $SUITEPATH | grep -E '[0-9]+_.*' )"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to match the numbers at the beginning? I.e. grep -E '^[0-9]+_.*' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto: right, but better to do this in a separate PR

exit 2
fi

SUITE="${SUITEPATH}/${TARGET}"
SUFFIX=$( echo $TARGET | cut -d_ -f2- )
BASENAME="e2e-pao"
EXTENSION="test"
OUTPUT="${BASENAME}-${SUFFIX}.${EXTENSION}"

echo "${SUITE} -> ${OUTDIR}/${OUTPUT}"
go test -c -v -o ${OUTDIR}/${OUTPUT} ${SUITE}
8 changes: 4 additions & 4 deletions hack/build-test-bin.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

set -e

PREFIX="pao-build-e2e-"
SUITEPATH="./test/e2e/performanceprofile/functests"
PREFIX="build-e2e-"
SUITEPATH="./test/e2e"
TARGET=$1

if [ -z "$TARGET" ]; then
echo "usage: $0 suite"
echo "example: $0 1_performance"
echo "example: $0 deferred"
exit 1
fi

Expand All @@ -22,7 +22,7 @@ fi

SUITE="${SUITEPATH}/${TARGET}"
SUFFIX=$( echo $TARGET | cut -d_ -f2- )
BASENAME="e2e-pao"
BASENAME="e2e"
EXTENSION="test"
OUTPUT="${BASENAME}-${SUFFIX}.${EXTENSION}"

Expand Down
3 changes: 3 additions & 0 deletions manifests/20-profile.crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ spec:
- jsonPath: .status.conditions[?(@.type=="Degraded")].status
name: Degraded
type: string
- jsonPath: .status.conditions[?(@.type=="Applied")].message
name: Message
type: string
- jsonPath: .metadata.creationTimestamp
name: Age
type: date
Expand Down
4 changes: 4 additions & 0 deletions pkg/apis/tuned/v1/tuned_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ const (
// TunedBootcmdlineAnnotationKey is a Node-specific annotation denoting kernel command-line parameters
// calculated by TuneD for the current profile applied to that Node.
TunedBootcmdlineAnnotationKey string = "tuned.openshift.io/bootcmdline"

// TunedDeferredUpdate request the tuned daemons to defer the update of the rendered profile
// until the next restart.
TunedDeferredUpdate string = "tuned.openshift.io/deferred"
)

/////////////////////////////////////////////////////////////////////////////////
Expand Down
7 changes: 6 additions & 1 deletion pkg/operator/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
}

klog.V(2).Infof("syncProfile(): Profile %s not found, creating one [%s]", profileMf.Name, computed.TunedProfileName)
profileMf.Annotations = util.ToggleDeferredUpdateAnnotation(profileMf.Annotations, computed.Deferred)
profileMf.Spec.Config.TunedProfile = computed.TunedProfileName
profileMf.Spec.Config.Debug = computed.Operand.Debug
profileMf.Spec.Config.TuneDConfig = computed.Operand.TuneDConfig
Expand Down Expand Up @@ -705,16 +706,20 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
}
}

anns := util.ToggleDeferredUpdateAnnotation(profile.Annotations, computed.Deferred)

// Minimize updates
if profile.Spec.Config.TunedProfile == computed.TunedProfileName &&
profile.Spec.Config.Debug == computed.Operand.Debug &&
reflect.DeepEqual(profile.Spec.Config.TuneDConfig, computed.Operand.TuneDConfig) &&
reflect.DeepEqual(profile.Spec.Profile, computed.AllProfiles) &&
util.HasDeferredUpdateAnnotation(profile.Annotations) == util.HasDeferredUpdateAnnotation(anns) &&
profile.Spec.Config.ProviderName == providerName {
klog.V(2).Infof("syncProfile(): no need to update Profile %s", nodeName)
return nil
}
profile = profile.DeepCopy() // never update the objects from cache
profile.Annotations = anns
profile.Spec.Config.TunedProfile = computed.TunedProfileName
profile.Spec.Config.Debug = computed.Operand.Debug
profile.Spec.Config.TuneDConfig = computed.Operand.TuneDConfig
Expand All @@ -727,7 +732,7 @@ func (c *Controller) syncProfile(tuned *tunedv1.Tuned, nodeName string) error {
if err != nil {
return fmt.Errorf("failed to update Profile %s: %v", profile.Name, err)
}
klog.Infof("updated profile %s [%s]", profile.Name, computed.TunedProfileName)
klog.Infof("updated profile %s [%s] (deferred=%v)", profile.Name, computed.TunedProfileName, util.HasDeferredUpdateAnnotation(profile.Annotations))

return nil
}
Expand Down
Loading