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

feat: support validate policy of opsrequest #8232

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ian-hui
Copy link
Contributor

@ian-hui ian-hui commented Sep 29, 2024

solve #8231

@ian-hui ian-hui requested review from wangyelei and a team as code owners September 29, 2024 10:27
@CLAassistant
Copy link

CLAassistant commented Sep 29, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines. label Sep 29, 2024
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Sep 29, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from eb5f7c9 to 23483db Compare September 29, 2024 10:29
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Sep 29, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 23483db to 9f847d9 Compare September 29, 2024 10:36
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Sep 29, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 9f847d9 to 5cdc5d0 Compare September 29, 2024 10:38
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Sep 29, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 5cdc5d0 to 12c2f78 Compare October 14, 2024 09:19
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Oct 14, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 12c2f78 to ae9ee0a Compare October 22, 2024 06:35
@github-actions github-actions bot added size/XXL Denotes a PR that changes 1000+ lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Oct 22, 2024
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Oct 22, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from ae9ee0a to 3ae1621 Compare November 4, 2024 07:05
@apecloud-bot apecloud-bot removed the pre-approve Fork PR Pre Approve Test label Nov 4, 2024
@github-actions github-actions bot added size/L Denotes a PR that changes 100-499 lines. and removed size/XXL Denotes a PR that changes 1000+ lines. labels Nov 4, 2024
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Nov 4, 2024
@wangyelei
Copy link
Contributor

fix the conflicts and make test error

@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 3ae1621 to c0c977c Compare November 5, 2024 11:30
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Nov 5, 2024
@ian-hui
Copy link
Contributor Author

ian-hui commented Nov 5, 2024

fix the conflicts and make test error

sorry that i forgot to push newest code. I have already push the newest version. plz check.

@wangyelei
Copy link
Contributor

@ian-hui
image

@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from c0c977c to 8798f3d Compare November 6, 2024 02:34
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Nov 6, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 8798f3d to 531b35d Compare November 6, 2024 11:45
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Nov 6, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 531b35d to 088b7ce Compare November 6, 2024 11:54
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Nov 6, 2024
@ian-hui ian-hui force-pushed the feat/support-opsrequest-validate-policy branch from 088b7ce to da2b678 Compare November 6, 2024 12:13
@apecloud-bot apecloud-bot removed the pre-approve Fork PR Pre Approve Test label Nov 6, 2024
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Nov 6, 2024
opsRes *OpsResource,
lastCompConfiguration opsv1alpha1.LastComponentConfiguration,
obj ComponentOpsInterface,
) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

better with the following function:

func (hs horizontalScalingOpsHandler) validateHorizontalScaling(
	opsRes *OpsResource,
	lastCompConfiguration opsv1alpha1.LastComponentConfiguration,
	horizontalScaling opsv1alpha1.HorizontalScaling,
) error {
	if opsRes.OpsRequest.Annotations[constant.IgnoreHscaleValidateAnnoKey] == "true" {
		return nil
	}
	if horizontalScaling.ScaleIn != nil {
		if err := hs.validateOnlineInstancesToOffline(lastCompConfiguration,
			horizontalScaling.ScaleIn.OnlineInstancesToOffline, opsRes.Cluster.Name, horizontalScaling.ComponentName); err != nil {
			return err
		}
	}
	if horizontalScaling.ScaleOut != nil {
		if err := hs.validateOfflineInstancesToOnline(lastCompConfiguration,
			horizontalScaling.ScaleOut.OfflineInstancesToOnline, horizontalScaling.ComponentName); err != nil {
			return err
		}
	}
	return nil
}

func (hs horizontalScalingOpsHandler) validateOnlineInstancesToOffline(
	lastCompConfiguration opsv1alpha1.LastComponentConfiguration,
	onlineInstancesToOffline []string,
	clusterName, componentName string) error {
	if len(onlineInstancesToOffline) == 0 {
		return nil
	}
	toOfflineSet := sets.New(onlineInstancesToOffline...)
	if len(toOfflineSet) < len(onlineInstancesToOffline) {
		return intctrlutil.NewFatalError("instances specified in onlineInstancesToOffline has duplicates")
	}
	currPodSet, err := intctrlcomp.GenerateAllPodNamesToSet(*lastCompConfiguration.Replicas, lastCompConfiguration.Instances,
		lastCompConfiguration.OfflineInstances, clusterName, componentName)
	if err != nil {
		return err
	}
	for _, onlineIns := range onlineInstancesToOffline {
		if _, ok := currPodSet[onlineIns]; !ok {
			return intctrlutil.NewFatalError(fmt.Sprintf(`instance "%s" specified in onlineInstancesToOffline is not onlinee`, onlineIns))
		}
	}
	return nil
}

func (hs horizontalScalingOpsHandler) validateOfflineInstancesToOnline(
	lastCompConfiguration opsv1alpha1.LastComponentConfiguration,
	offlineInstancesToOnline []string,
	componentName string) error {
	if len(offlineInstancesToOnline) == 0 {
		return nil
	}
	toOnlineSet := sets.New(offlineInstancesToOnline...)
	if len(toOnlineSet) < len(offlineInstancesToOnline) {
		return intctrlutil.NewFatalError("instances specified in offlineInstancesToOnline has duplicates")
	}
	offlineInstanceSet := sets.New(lastCompConfiguration.OfflineInstances...)
	for _, offlineIns := range offlineInstancesToOnline {
		if _, ok := offlineInstanceSet[offlineIns]; !ok {
			return intctrlutil.NewFatalError(fmt.Sprintf(`cannot find the offline instance "%s" in component "%s" for scaleOut operation`, offlineIns, componentName))
		}
	}
	return nil
}

@@ -26,7 +26,6 @@ import (
"strings"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/intstr"
Copy link
Contributor

Choose a reason for hiding this comment

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

rebase main branch

}
}
return diff
}
Copy link
Contributor

Choose a reason for hiding this comment

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

remove it.

insTplName := appsv1.GetInstanceTemplateName(opsRes.Cluster.Name, horizontalScaling.ComponentName, insName)
onlineInsCountMap[insTplName]++
}
onlineInsCountMap := opsRes.OpsRequest.CountOfflineOrOnlineInstances(opsRes.Cluster.Name, horizontalScaling.ComponentName, scaleOut.OfflineInstancesToOnline)
Copy link
Contributor

Choose a reason for hiding this comment

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

revert it.

There is a scenario where the original offlineInstances were [pod1, pod3, pod10], and the offlineInstancesToOnline was pod10. However, the current replicas count is 2, which means that pod10 will not actually go online.

Copy link
Contributor

Choose a reason for hiding this comment

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

or, calculate the real onlineInstances in filterHorizontalScalingSpec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pre-approve Fork PR Pre Approve Test size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants