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

add workload runtime support for aks #3896

Merged
merged 5 commits into from
Sep 29, 2021
Merged

Conversation

alexeldeib
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@yonzhan
Copy link
Collaborator

yonzhan commented Sep 16, 2021

aks

@alexeldeib
Copy link
Contributor Author

seems like 429? will retry tomorrow :/

text: az aks nodepool add -g MyResourceGroup -n nodepool1 --cluster-name MyManagedCluster --os-sku Ubuntu
- name: Create a nodepool which can run wasm workloads.
text: az aks nodepool add -g MyResourceGroup -n nodepool1 --cluster-name MyManagedCluster --workload-runtim WasmWasi
Copy link
Member

Choose a reason for hiding this comment

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

typo --workload-runtim

@@ -141,6 +142,7 @@ def load_arguments(self, _):
c.argument('assign_kubelet_identity', type=str, validator=validate_assign_kubelet_identity)
c.argument('disable_local_accounts', action='store_true')
c.argument('yes', options_list=['--yes', '-y'], help='Do not prompt for confirmation.', action='store_true')
c.argument('workload_runtime', arg_type=get_enum_type([CONST_WORKLOAD_RUNTIME_OCI_CONTAINER, CONST_WORKLOAD_RUNTIME_WASM_WASI]))
Copy link
Member

@FumingZhang FumingZhang Sep 16, 2021

Choose a reason for hiding this comment

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

I suppose it would be better if you could create a global variable such as workload_runtimes = [CONST_WORKLOAD_RUNTIME_OCI_CONTAINER, CONST_WORKLOAD_RUNTIME_WASM_WASI], then set this variable to get_enum_type, which would avoid forgetting to modify the optional parameters of create or nodepool add in the future.

Copy link
Member

Choose a reason for hiding this comment

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

From help message, I suppose we should add default=CONST_WORKLOAD_RUNTIME_OCI_CONTAINER?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

better?

src/aks-preview/azext_aks_preview/_params.py Outdated Show resolved Hide resolved
@alexeldeib
Copy link
Contributor Author

@FumingZhang waiting for anything here besides re-running live tests?

@zhoxing-ms
Copy link
Contributor

@alexeldeib Please write those changes into the HISTORY.rst

By the way, do you need to release a new version for these changes? If so, please also update the setup.py

@zhoxing-ms
Copy link
Contributor

In addition, could you please address the live test issue?

@alexeldeib
Copy link
Contributor Author

please write those changes into the HISTORY.rst
added

By the way, do you need to release a new version for these changes? If so, please also update the setup.py

yes please :) bumped to 0.5.34

In addition, could you please address the live test issue?

@zhoxing-ms these don't seem related, since the newly added test is a preview feature and doesn't run in the live pipeline. the previous failures I saw seemed throttling related (ARM 429). the new commits triggered a fresh build, hopefully that works. in the future, is there a good way to manually force a rerun? I did not see one.

@alexeldeib
Copy link
Contributor Author

@zhoxing-ms @hbeberman looks like the failing test is related to mariner, I suspect due to a known issue with extensions @hbeberman discovered this week. I'm not sure whether we should disable the live mariner test here, or wait for the RP fix to unblock existing PRs?

@FumingZhang
Copy link
Member

Yes, I can help confirm that the failed test case is due to compatibility issues with CBLMariner and has nothing to do with the changes in this PR. So I suppose we could bypass the check and merge the PR for this time.

I am working on a PR #3927 to exclude cases like this.

@zhoxing-ms zhoxing-ms merged commit 33ec455 into Azure:main Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants