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

Jump to bottom

Fixing script bug #3651

Merged
merged 6 commits into from
Jul 18, 2023
+74 −50
Merged

Fixing script bug #3651

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9881911
Fixing script bug
wilmer05 Jul 18, 2023
dd4fa8b
adding PR feedback
wilmer05 Jul 18, 2023
4596d2a
Removing fix for tests
wilmer05 Jul 18, 2023
6bf359c
Rolling back safe_load
wilmer05 Jul 18, 2023
8356392
Fixing spark load
wilmer05 Jul 18, 2023
e88f9e7
Removing ci workflow
wilmer05 Jul 18, 2023
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
2 changes: 2 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ jobs:
DOCKER_REGISTRY: ""
steps:
- uses: actions/checkout@v2
- name: Install docker-compose
run: sudo apt-get update && sudo apt-get install -y docker-compose=1.25.0-1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Install docker-compose
run: sudo apt-get update && sudo apt-get install -y docker-compose=1.25.0-1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed, thnx for the catch

- uses: actions/setup-python@v2
with:
python-version: 3.7
Expand Down
2 changes: 1 addition & 1 deletion paasta_tools/cli/cmds/spark_run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1203,7 +1203,7 @@ def paasta_spark_run(args):
document = POD_TEMPLATE.format(
spark_pod_label=limit_size_with_hash(f"exec-{app_base_name}"),
)
parsed_pod_template = yaml.load(document)
parsed_pod_template = yaml.full_load(document)
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be safe_load if anything :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

with open(pod_template_path, "w") as f:
yaml.dump(parsed_pod_template, f)

Expand Down
22 changes: 14 additions & 8 deletions paasta_tools/kubernetes_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
from kubernetes.client import V1Affinity
from kubernetes.client import V1AWSElasticBlockStoreVolumeSource
from kubernetes.client import V1beta1CustomResourceDefinition
from kubernetes.client import V1beta1CustomResourceDefinitionList
from kubernetes.client import V1beta1PodDisruptionBudget
from kubernetes.client import V1beta1PodDisruptionBudgetSpec
from kubernetes.client import V1Capabilities
Expand Down Expand Up @@ -3859,27 +3860,32 @@ def mode_to_int(mode: Optional[Union[str, int]]) -> Optional[int]:


def update_crds(
apiextensions: Union[
kube_client.ApiextensionsV1Api, kube_client.ApiextensionsV1beta1Api
],
kube_client: KubeClient,
desired_crds: Collection[
Union[V1CustomResourceDefinition, V1beta1CustomResourceDefinition]
],
existing_crds: V1CustomResourceDefinitionList,
existing_crds: Union[
V1CustomResourceDefinitionList, V1beta1CustomResourceDefinitionList
],
) -> bool:
success = True
for desired_crd in desired_crds:
existing_crd = None
for crd in existing_crds.items:
if crd.metadata.name == desired_crd.metadata["name"]:
existing_crd = crd
break

try:

if "beta" in desired_crd.api_version:
Copy link
Member

Choose a reason for hiding this comment

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

should we check for equality rather than substring? e.g., if "v1beta1" == desired_crd.api_version to make sure we don't accidentally leave future us a timebomb if we somehow forget to clean this up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that is not the version, the version is a longer string if I'm not mistaken, that's why I left it like "in" instead of matching the whole thing

Copy link
Member

Choose a reason for hiding this comment

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

ack - my understanding is that this should be something like apiextensions/v1beta1 or apiextensions/v1, but as long as we're diligent about cleaning up once this is done then it's not really a big deal :)

apiextensions = kube_client.apiextensions_v1_beta1
else:
apiextensions = kube_client.apiextensions

if existing_crd:
desired_crd.metadata[
"resourceVersion"
] = existing_crd.metadata.resource_version

apiextensions.replace_custom_resource_definition(
name=desired_crd.metadata["name"], body=desired_crd
)
Expand All @@ -3901,9 +3907,9 @@ def update_crds(
f"status: {exc.status}, reason: {exc.reason}"
)
log.debug(exc.body)
success = False
return False
Copy link
Member

Choose a reason for hiding this comment

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

this is a slight behavior change (previously, we'd attempt to apply all CRDs regardless of whether any of them failed) - do we potentially rely on the previous behavior anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, the reason is: we need to succeed in ever CRD now, since now to the list of desired_crd we send all of those CRDs that need to be updated and not that "may get updated" since we know now which is the apiversion of each


return success
return True


def sanitise_label_value(value: str) -> str:
Expand Down
78 changes: 50 additions & 28 deletions paasta_tools/setup_kubernetes_crd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import service_configuration_lib
from kubernetes.client import V1beta1CustomResourceDefinition
from kubernetes.client import V1CustomResourceDefinition
from kubernetes.client.exceptions import ApiException

from paasta_tools.kubernetes_tools import KubeClient
from paasta_tools.kubernetes_tools import paasta_prefixed
Expand Down Expand Up @@ -101,43 +102,64 @@ def setup_kube_crd(
services: Sequence[str],
soa_dir: str = DEFAULT_SOA_DIR,
) -> bool:
for apiextension, crd_class in [
(kube_client.apiextensions, V1CustomResourceDefinition),
(kube_client.apiextensions_v1_beta1, V1beta1CustomResourceDefinition),
]:
existing_crds = apiextension.list_custom_resource_definition(
label_selector=paasta_prefixed("service")
)
existing_crds = kube_client.apiextensions.list_custom_resource_definition(
label_selector=paasta_prefixed("service")
)

desired_crds = []
for service in services:
crd_config = service_configuration_lib.read_extra_service_information(
service, f"crd-{cluster}", soa_dir=soa_dir
# This step can fail in k8s 1.22 since this version is not existing anymore
# we need to support this for the transition
try:
existing_crds_v1_beta1 = (
nemacysts marked this conversation as resolved.
Show resolved Hide resolved
kube_client.apiextensions_v1_beta1.list_custom_resource_definition(
label_selector=paasta_prefixed("service")
)
)
except ApiException as e:
existing_crds_v1_beta1 = []
log.debug(e)
Copy link
Member

Choose a reason for hiding this comment

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

will this debug statment be deleted or do we want to reword this message to log.debug("Listing CRDs with apiextensions/v1beta1 not supported on this cluster, falling back to v1")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that log message, will change it now


desired_crds = []
desired_crds_v1_beta_1 = []
for service in services:
crd_config = service_configuration_lib.read_extra_service_information(
service, f"crd-{cluster}", soa_dir=soa_dir
)
if not crd_config:
log.info("nothing to deploy")
continue

metadata = crd_config.get("metadata", {})
if "labels" not in metadata:
metadata["labels"] = {}
metadata["labels"]["yelp.com/paasta_service"] = service
metadata["labels"][paasta_prefixed("service")] = service

if "beta" in crd_config["apiVersion"]:
Copy link
Member

Choose a reason for hiding this comment

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

same as above, should we maybe be checking for version equality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added now

desired_crd = V1beta1CustomResourceDefinition(
api_version=crd_config.get("apiVersion"),
kind=crd_config.get("kind"),
metadata=metadata,
spec=crd_config.get("spec"),
)
if not crd_config:
log.info("nothing to deploy")
continue

metadata = crd_config.get("metadata", {})
if "labels" not in metadata:
metadata["labels"] = {}
metadata["labels"]["yelp.com/paasta_service"] = service
metadata["labels"][paasta_prefixed("service")] = service
desired_crd = crd_class(
desired_crds_v1_beta_1.append(desired_crd)
else:
desired_crd = V1CustomResourceDefinition(
api_version=crd_config.get("apiVersion"),
kind=crd_config.get("kind"),
metadata=metadata,
spec=crd_config.get("spec"),
)
desired_crds.append(desired_crd)

if update_crds(
apiextensions=apiextension,
desired_crds=desired_crds,
existing_crds=existing_crds,
):
return True
return False
return update_crds(
kube_client=kube_client,
desired_crds=desired_crds,
existing_crds=existing_crds,
) and update_crds(
kube_client=kube_client,
desired_crds=desired_crds_v1_beta_1,
existing_crds=existing_crds_v1_beta1,
)


if __name__ == "__main__":
Expand Down
22 changes: 8 additions & 14 deletions paasta_tools/setup_kubernetes_internal_crd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,20 +140,14 @@ def main() -> None:
def setup_kube_internal_crd(
kube_client: KubeClient,
) -> bool:
for apiextension in [
kube_client.apiextensions,
kube_client.apiextensions_v1_beta1,
]:
existing_crds = apiextension.list_custom_resource_definition(
label_selector=paasta_prefixed("internal")
)
if update_crds(
apiextensions=apiextension,
desired_crds=INTERNAL_CRDS,
existing_crds=existing_crds,
):
return True
return False
existing_crds = kube_client.apiextensions.list_custom_resource_definition(
label_selector=paasta_prefixed("internal")
)
return update_crds(
kube_client=kube_client,
desired_crds=INTERNAL_CRDS,
existing_crds=existing_crds,
)


if __name__ == "__main__":
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ python-iptables==1.0.1
python-utils==2.0.1
pytimeparse==1.1.5
pytz==2016.10
pyyaml==5.4.1
pyyaml==6.0.1
repoze.lru==0.6
requests==2.25.0
requests-cache==0.6.3
Expand Down
3 changes: 0 additions & 3 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ basepython = python3.7
whitelist_externals = bash
deps =
urllib3<2.0
docker-compose=={[tox]docker_compose_version}
Copy link
Member

Choose a reason for hiding this comment

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

we'll need to figure out what to do internally as well: we'll need the docker-compose deb installed on jenkins + devboxes before we merge this

Copy link
Member

Choose a reason for hiding this comment

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

alternatively, we can continue using docker-compose internally since our internal setup is guaranteed to only hit our pypi + we have things setup internally to grab wheels

...or maybe do the potentially painful thing and start removing the usage of pip-custom-platform in this repo so that we hopefully grab the pypi.org wheels and skip these shenanigans?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how can I achieve the first option? I need to deploy these changes AFAP since is blocking me from upgrading to k8s 1.22

setenv =
passenv =
KIND_CLUSTER
Expand Down Expand Up @@ -118,8 +117,6 @@ commands =
[testenv:example_cluster]
changedir=example_cluster/
passenv = DOCKER_TLS_VERIFY DOCKER_HOST DOCKER_CERT_PATH
deps =
docker-compose=={[tox]docker_compose_version}
commands =
docker-compose down
docker-compose --verbose build --build-arg DOCKER_REGISTRY={env:DOCKER_REGISTRY:docker-dev.yelpcorp.com/} --build-arg PIP_INDEX_URL={env:PIP_INDEX_URL:https://pypi.yelpcorp.com/simple}
Expand Down
Loading