-
Notifications
You must be signed in to change notification settings - Fork 3k
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
AKS/ACI - Add install-connector and remove-connector commands #4996
Conversation
View a preview at https://prompt.ws/r/Azure/azure-cli/4996 |
if location is None: | ||
location = rgkaci.location # pylint:disable=no-member | ||
# Ensure that the SPN exists | ||
principal_obj = _ensure_service_principal(service_principal, client_secret, subscription_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a concern about it: _ensure_service_principal
implicitly grants subscription scope contributor to the spn. I dont think you want it.
Suggest to remove below code from _ensure_service_principal
all together as it's not needed:
# add role first before save it
if not _add_role_assignment('Contributor', service_principal):
logger.warning('Could not create a service principal with the right permissions. '
'Are you an Owner on this project?')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weinong You are asking @julienstroheker to remove code thats not even part of the commit. If theres a concern with another module/function i suggest submitting a PR or issue for that code. This is also an overall problem that should be addressed as an issue regarding the functionality of that method. Which I agree, we should not be scoping to an entire sub, but we are already doing that for acs/aks, thus this should be its own issue and un-related to thi pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@listonb np! we can fix that separately.
I can't see where the command line options and docs are coming from in the Python, so I'll leave my notes here:
|
alright so --os for os types with the options windows, linux, both |
Open question about defaults - for simplicity Linux, for coolness Windows + Linux. |
@itowlson Thanks for the comments!
|
@julienstroheker Thanks for the clarifications and for your thoughtful responses. A couple of picky nits around the name and OS stuff, possibly due to my lack of understanding:
|
Make senses ? |
try: | ||
if windows: | ||
drain_node = subprocess.check_output( | ||
["kubectl", "drain", "aci-connector-0", "--force"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use the "name" parameter instead of aci-connector-x
["kubectl", "drain", "aci-connector-0", "--force"], | ||
universal_newlines=True) | ||
drain_node = subprocess.check_output( | ||
["kubectl", "drain", "aci-connector-1", "--force"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. show we use {name}-win, {name}-linux as the node name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not supported in the current connector codebase, we cannot specify a custom node name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see @itowlson last comment
@julienstroheker That's cool, I didn't realise the Regarding why we need the OS type on remove, thanks for the offline conversation - I'll document it here so others have the information too. The issue, as I understand it, is:
Let me know any errors or important omissions and I'll edit this comment so it can act as a reference. |
@troydai not sure who I need from the CLI team to get this merged |
@devigned @JasonRShaver Can we get a review of this? thx!! |
Is it possible to have |
:type chart_url: str | ||
:param os_type: Os type target, could be Windows, Linux or Both. | ||
:type os_type: str | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All help should be defined in params.py and _help.py not in the docstring of the commands.
--os-type
: In params.py, define a choice_list of this param with possible values of ['Windows', 'Linux', 'Both']
--chart-url
: In params.py, you can specify the default value as a string default='...'
helm_not_installed = "Error : Helm not detected, please verify if it is installed." | ||
image_tag = 'latest' | ||
if chart_url is None: | ||
url_chart = "https://github.com/Azure/aci-connector-k8s/raw/master/charts/aci-connector.tgz" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you define chart_url in params.py with a default, this is not needed.
:type os_type: str | ||
""" | ||
from subprocess import PIPE, Popen | ||
helm_not_installed = "Error : Helm not detected, please verify if it is installed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the 'Error: ' prefix.
CLIErrors are already handled by the CLI appropriately (red if color available or with the ERROR: prefix that the logger provides).
profile = Profile() | ||
_, _, tenant_id = profile.get_login_credentials() | ||
# Check if we want the windows connector | ||
if (os_type == 'Windows') or (os_type == 'Both'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Define 'Windows', 'Both', 'Linux' as constants and this should be a case-insensitive comparison.
:param connector_name: The name for the ACI Connector | ||
:type connector_name: str | ||
:param graceful: Mention if you want to drain/uncordon your aci-connector to move your applications | ||
running on ACI to your others nodes. Default : False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you move the param descriptions to params.py, you can define the default there.
Consider making --graceful a flag so if --graceful
it set, your graceful
variable will be set to true, if not, it will be set to false. This can be done through params.py.
# Just do the get, we don't need the result, it will error out if the group doesn't exist. | ||
if graceful: | ||
logger.warning('Graceful option selected, will try to drain the node first') | ||
kubectl_not_installed = "Error : Kubectl not detected, please verify if it is installed." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to prefix with Error :
except OSError: | ||
raise CLIError(kubectl_not_installed) | ||
try: | ||
if (os_type == 'Windows') or (os_type == 'Both'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, should be a case-insensitive comparison.
@derekbekoe Thanks for the review ! I am on it ! |
@derekbekoe feel free to look at the new commits. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more comments.
short-summary: Deploy the ACI-Connector to a AKS cluster. | ||
examples: | ||
- name: Install the ACI-Connector to an AKS Cluster. | ||
text: az aks install-connector --name MyManagedCluster --resource-group MyResourceGroup --connector_name MyConnector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument name should be --connector-name
not ``--connector_name`.
Same elsewhere in this help file.
- name: Install the ACI-Connector for Windows and Linux to an AKS Cluster. | ||
text: az aks install-connector --name MyManagedCluster --resource-group MyResourceGroup --connector_name MyConnector --os-type Both | ||
- name: Install the ACI-Connector using specific SPN. | ||
text: az aks install-connector --name MyManagedCluster --resource-group MyResourceGroup --connector_name MyConnector --service_principal <SPN_ID> --client_secret<SPN_SECRET> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There would be a space after --client-secret
I believe.
register_cli_argument('aks install-connector', 'os_type', help='The OS type of the connector', **enum_choice_list(aci_conector_os_type)) | ||
register_cli_argument('aks install-connector', 'chart_url', help='URL to the chart', default=aci_connector_chart_url) | ||
register_cli_argument('aks remove-connector', 'os_type', help='The OS type of the connector', **enum_choice_list(aci_conector_os_type)) | ||
register_cli_argument('aks remove-connector', 'graceful', help='Mention if you want to drain/uncordon your aci-connector to move your applications', default=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this to actually be a flag, use action='store_true'
then remove the default.
Right now, you'd have to specify --graceful True
which isn't what is wanted.
:type chart_url: str | ||
:param os_type: Os type target. | ||
:type os_type: str | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete this whole docstring.
:type graceful: bool | ||
:param os_type: Os type target. | ||
:type os_type: str | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can remove this whole docstring.
@@ -72,6 +72,10 @@ def _get_feature_in_preview_message(): | |||
|
|||
orchestratorTypes = ["Custom", "DCOS", "Kubernetes", "Swarm", "DockerCE"] | |||
|
|||
aci_conector_os_type = ['Windows', 'Linux', 'Both'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo connector
@derekbekoe Thanks again ! I applied your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me, my comments are optional. I haven't been able to test it interactively though.
profile = Profile() | ||
_, _, tenant_id = profile.get_login_credentials() | ||
# Check if we want the windows connector | ||
if (os_type == 'windows') or (os_type == 'both'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Optional) you could simplify this a bit:
if os_type in ['windows', 'both']:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do the change, thanks
@@ -14,7 +14,7 @@ | |||
logger.warn("Wheel is not available, disabling bdist_wheel hook") | |||
cmdclass = {} | |||
|
|||
VERSION = "2.0.20" | |||
VERSION = "2.0.21" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're supposed to leave this alone and let the maintainers increment it--at least I had a previous PR where that was the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, I did it because it failed the CI. Do you want me to put it back version .20?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a process change recently that if yours is the first one since the latest release, you should bump up the version here and in the HISTORY.RST; otherwise CI will fail
except OSError: | ||
raise CLIError(kubectl_not_installed) | ||
try: | ||
if (os_type == 'windows') or (os_type == 'both'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above--could use in
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do the change too.
_, browse_path = tempfile.mkstemp() | ||
aks_get_credentials(client, resource_group, name, admin=False, path=browse_path) | ||
# Validate if the RG exists | ||
# Just do the get, we don't need the result, it will error out if the group doesn't exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 comments are invalid
profile = Profile() | ||
_, _, tenant_id = profile.get_login_credentials() | ||
# Check if we want the windows connector | ||
if (os_type == 'windows') or (os_type == 'both'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cross check whether you need to use case-insensitive comparison, considering you have defined the enum list with the first character in capital aci_connector_os_type = ['Windows', 'Linux', 'Both']
drain_node = subprocess.check_output( | ||
["kubectl", "drain", "aci-connector-0", "--force"], | ||
universal_newlines=True) | ||
drain_node = subprocess.check_output( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest use a loop to avoid code dupes, something like
nodes = ['-0', '-1'] if os_type.lower() in ['windows', 'both'] else ['']
for n in nodes:
drain_node = subprocess.check_output(["kubectl", "drain", "aci-connector{}".format(n), "--force"], universal_newlines=True)
raise CLIError('Could not find the node, make sure you' + | ||
'are using the correct --os-type option: {}'.format(err)) | ||
if drain_node: | ||
# remove the "pods/" prefix from the name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment might be wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Same 2 comments as before. Up to you if you want to address.
--service-principal is not specified, the secret is auto-generated for you | ||
and stored in ${HOME}/.azure/ directory. | ||
:type client_secret: str | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still just remove this block of comments.
Everything should be defined in params.py already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, let me move all of it in the params.py
file to make it cleaner.
:type resource_group: str | ||
:param connector_name: The name for the ACI Connector | ||
:type connector_name: str | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still just remove this block of comments.
Everything should be defined in params.py already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, let me move all of it in the params.py
file to make it cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more comment. Please do enough manual testing, particularly for both windows and linux, and let me know when it is ready to merge.
raise CLIError('Could not find the node, make sure you' + | ||
'are using the correct --os-type option: {}'.format(err)) | ||
if drain_node: | ||
drain_node = str(drain_node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drain_node
is not used any more in this method, so why not simplify like
if not drain_node:
raise CLIError("Couldn't find the node.")
* [AKS] refactor to use Knack framework * AKS/ACI - Add install-connector and remove-connector commands (#4996) * Addressed some review comments. * Use common SSH key validator. * Rename commands loader to avoid a conflict. * Enable tests for the ACS (AKS) module.
Add
az aks install-connector
andaz aks remove-connector
commandsThis checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Command Guidelines
(see Authoring Command Modules)