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

fix: always reset overlay data when the base value was set on the process obj #1745

Merged
merged 2 commits into from
Nov 21, 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
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from typing import Dict, Iterable, List, Set, Tuple, Union

from paasng.platform.bkapp_model import fieldmgr
from paasng.platform.bkapp_model.entities import AutoscalingOverlay, ReplicasOverlay, ResQuotaOverlay
from paasng.platform.bkapp_model.entities import AutoscalingOverlay, Process, ReplicasOverlay, ResQuotaOverlay
from paasng.platform.bkapp_model.models import ModuleProcessSpec, ProcessSpecEnvOverlay
from paasng.platform.modules.models import Module
from paasng.utils.structure import NotSetType
Expand All @@ -31,27 +31,48 @@


def sync_env_overlays_replicas(
Copy link
Collaborator

@jamesgetx jamesgetx Nov 21, 2024

Choose a reason for hiding this comment

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

现在的特殊处理策略分散在了 sync_xxx 逻辑中。可以考虑在sync 之前预处理 bk app 模型(比如补全一些 env overlay配置),这样 sync 逻辑职责比较单一,同步规则稳定

module: Module, overlay_replicas: List[ReplicasOverlay] | NotSetType, manager: fieldmgr.FieldMgrName
module: Module,
overlay_replicas: List[ReplicasOverlay] | NotSetType,
manager: fieldmgr.FieldMgrName,
processes: List[Process] | None = None,
piglei marked this conversation as resolved.
Show resolved Hide resolved
) -> CommonSyncResult:
"""Sync replicas overlay data to db."""
syncer = OverlayDataSyncer(algo=ReplicasSyncerFieldAlgo())
return syncer.sync(module, overlay_replicas, manager)
if processes is None:
proc_value_is_set = None
piglei marked this conversation as resolved.
Show resolved Hide resolved
else:
proc_value_is_set = {p.name: not isinstance(p.replicas, NotSetType) for p in processes}
return syncer.sync(module, overlay_replicas, manager, proc_value_is_set)


def sync_env_overlays_res_quotas(
module: Module, overlay_res_quotas: List[ResQuotaOverlay] | NotSetType, manager: fieldmgr.FieldMgrName
module: Module,
overlay_res_quotas: List[ResQuotaOverlay] | NotSetType,
manager: fieldmgr.FieldMgrName,
processes: List[Process] | None = None,
) -> CommonSyncResult:
"""Sync res_quota overlay data to db."""
syncer = OverlayDataSyncer(algo=ResQuotasSyncerFieldAlgo())
return syncer.sync(module, overlay_res_quotas, manager)
if processes is None:
proc_value_is_set = None
else:
proc_value_is_set = {p.name: not isinstance(p.res_quota_plan, NotSetType) for p in processes}
return syncer.sync(module, overlay_res_quotas, manager, proc_value_is_set)


def sync_env_overlays_autoscalings(
module: Module, overlay_autoscalings: List[AutoscalingOverlay] | NotSetType, manager: fieldmgr.FieldMgrName
module: Module,
overlay_autoscalings: List[AutoscalingOverlay] | NotSetType,
manager: fieldmgr.FieldMgrName,
processes: List[Process] | None = None,
) -> CommonSyncResult:
"""Sync autoscaling overlay data to db model"""
syncer = OverlayDataSyncer(algo=AutoscalingSyncerFieldAlgo())
return syncer.sync(module, overlay_autoscalings, manager)
if processes is None:
proc_value_is_set = None
else:
proc_value_is_set = {p.name: not isinstance(p.autoscaling, NotSetType) for p in processes}
return syncer.sync(module, overlay_autoscalings, manager, proc_value_is_set)


def clean_empty_overlays(module):
Expand Down Expand Up @@ -83,11 +104,17 @@ def sync(
module: Module,
items: Iterable[Union[ReplicasOverlay, ResQuotaOverlay, AutoscalingOverlay]] | NotSetType,
manager: fieldmgr.FieldMgrName,
proc_value_is_set: dict[str, bool] | None,
) -> CommonSyncResult:
"""Sync overlay data to the db."""
"""Sync overlay data to the db.

:param proc_value_is_set: An optional dict to indicate if the base value was
set on the process object. When the `items` is not set and the value of
the process object was set, the overlay data will always be reset.
"""
ret = CommonSyncResult()
if isinstance(items, NotSetType):
return self._sync_notset(module, manager)
return self._sync_notset(module, manager, proc_value_is_set)

# Build the index of existing data first to clean data later.
existing_specs, existing_index = self._build_specs_and_index(module)
Expand All @@ -113,7 +140,9 @@ def sync(
ret.deleted_num += 1
return ret

def _sync_notset(self, module: Module, manager: fieldmgr.FieldMgrName) -> CommonSyncResult:
def _sync_notset(
self, module: Module, manager: fieldmgr.FieldMgrName, proc_value_is_set: dict[str, bool] | None
) -> CommonSyncResult:
"""Sync when the given data is not set."""
ret = CommonSyncResult()

Expand All @@ -123,8 +152,9 @@ def _sync_notset(self, module: Module, manager: fieldmgr.FieldMgrName) -> Common

# Reset existing data
for (proc, env), pk in existing_index.items():
base_value_is_set = proc_value_is_set and proc_value_is_set.get(proc)
# Do not reset the data if the environment is not managed by the current manager.
if (proc, env) in not_managed_envs:
if (proc, env) in not_managed_envs and not base_value_is_set:
continue

ProcessSpecEnvOverlay.objects.update_or_create(pk=pk, defaults=self.algo.get_empty_values())
Expand Down
6 changes: 3 additions & 3 deletions apiserver/paasng/paasng/platform/bkapp_model/importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,9 @@ def import_bkapp_spec_entity(module: Module, spec_entity: v1alpha2_entity.BkAppS
sync_observability(module, spec_entity.observability)

# NOTE: Must import the processes first to create the ModuleProcessSpec objs
sync_env_overlays_replicas(module, overlay_replicas, manager)
sync_env_overlays_res_quotas(module, overlay_res_quotas, manager)
sync_env_overlays_autoscalings(module, overlay_autoscaling, manager)
sync_env_overlays_replicas(module, overlay_replicas, manager, spec_entity.processes)
sync_env_overlays_res_quotas(module, overlay_res_quotas, manager, spec_entity.processes)
sync_env_overlays_autoscalings(module, overlay_autoscaling, manager, spec_entity.processes)

clean_empty_overlays(module)

Expand Down
34 changes: 16 additions & 18 deletions apiserver/paasng/tests/paasng/platform/bkapp_model/test_importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,26 +240,12 @@ class TestReplicasOverlay:
@pytest.fixture()
def base_manifest_with_overlay(self, base_manifest):
d = copy.deepcopy(base_manifest)
d["spec"]["envOverlay"] = {
"replicas": [
{
"envName": "stag",
"process": "web",
"count": "3",
}
]
}
d["spec"]["envOverlay"] = {"replicas": [{"envName": "stag", "process": "web", "count": "3"}]}
return d

def test_invalid_replicas_input(self, bk_module, base_manifest):
base_manifest["spec"]["envOverlay"] = {
"replicas": [
{
"envName": "stag",
"process": "web",
"count": "not_a_number",
}
]
"replicas": [{"envName": "stag", "process": "web", "count": "not_a_number"}]
}
with pytest.raises(ManifestImportError) as e:
import_manifest_app_desc(bk_module, base_manifest)
Expand All @@ -278,13 +264,25 @@ def test_reset_by_not_providing_value(self, bk_module, base_manifest, base_manif
proc_spec = ModuleProcessSpec.objects.get(module=bk_module, name="web")
assert proc_spec.get_target_replicas("stag") == 1, "The overlay data should be reset"

def test_not_reset_when_manager_different(self, bk_module, base_manifest, base_manifest_with_overlay):
def test_not_reset_when_manager_different(self, bk_module, manifest_no_replicas, base_manifest_with_overlay):
import_manifest_app_desc(bk_module, base_manifest_with_overlay)
import_manifest(bk_module, base_manifest, manager=fieldmgr.FieldMgrName.WEB_FORM)
import_manifest(bk_module, manifest_no_replicas, manager=fieldmgr.FieldMgrName.WEB_FORM)

proc_spec = ModuleProcessSpec.objects.get(module=bk_module, name="web")
assert proc_spec.get_target_replicas("stag") == 3, "The overlay data should remain as it is"

def test_proc_replicas_reset_overlay_managed_by_other(self, bk_module, base_manifest, base_manifest_with_overlay):
"""When the replicas field is set on the process object, the overlay should
always be reset even if no overlay data is provided.
"""
import_manifest(bk_module, base_manifest_with_overlay, manager=fieldmgr.FieldMgrName.WEB_FORM)
proc_spec = ModuleProcessSpec.objects.get(module=bk_module, name="web")
assert proc_spec.get_target_replicas("stag") == 3

import_manifest_app_desc(bk_module, base_manifest)
proc_spec.refresh_from_db()
assert proc_spec.get_target_replicas("stag") == 1


class TestAutoscaling:
def test_normal(self, bk_module, base_manifest):
Expand Down