-
Notifications
You must be signed in to change notification settings - Fork 33
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
Added eksengine to engines/__init__.py #315
base: master
Are you sure you want to change the base?
Conversation
Added boto3 to setup.py requirements Added eks engine Added eks kubeconfig template
kqueen/engines/eks.py
Outdated
'ACTIVE': config.get('CLUSTER_OK_STATE'), | ||
'DELETING': config.get('CLUSTER_DEPROVISIONING_STATE'), | ||
'FAILED': config.get('CLUSTER_ERROR_STATE'), | ||
'UPDATED': config.get('CLUSTER_UPDATING_STATE') |
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.
'UPDATING'
?
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.
@ekhomyakova I suppose those names comes from EKS, but I don't see updating in the list at all, so may be we can just remove updated from the list
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 what i know the engine requires an updating state, so either we leave it or we need to put it to unknown
kqueen/engines/eks.py
Outdated
'number': True | ||
} | ||
}, | ||
'roleArn': { |
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.
It's better to use snake_case everywhere
kqueen/engines/eks.py
Outdated
'required': True | ||
} | ||
}, | ||
'subnetid': { |
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.
subnet_id
kqueen/engines/eks.py
Outdated
'required': True | ||
} | ||
}, | ||
'securitygroupid': { |
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.
security_group_id
or securitygroup_id
kqueen/engines/eks.py
Outdated
except Exception as e: | ||
msg = 'Creating cluster {} failed with the following reason:'.format(self.cluster.id) | ||
logger.exception(msg) | ||
return False, msg |
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, return only "e" instead of msg, since message will be huge and looks like:
Error occurred with the following reason: Creating cluster failed with the following reason:'
kqueen/engines/eks.py
Outdated
def engine_status(cls, **kwargs): | ||
try: | ||
aws_access_key = kwargs.get('aws_access_key', '') | ||
aws_secret_key = kwargs.get('aws_secret_key', '') |
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.
did you mean 'aws_secret_access_key'?
kqueen/engines/eks.py
Outdated
'ACTIVE': config.get('CLUSTER_OK_STATE'), | ||
'DELETING': config.get('CLUSTER_DEPROVISIONING_STATE'), | ||
'FAILED': config.get('CLUSTER_ERROR_STATE'), | ||
'UPDATED': config.get('CLUSTER_UPDATING_STATE') |
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.
@ekhomyakova I suppose those names comes from EKS, but I don't see updating in the list at all, so may be we can just remove updated from the list
kqueen/engines/eks.py
Outdated
""" | ||
Implementation of :func:`~kqueen.engines.base.BaseEngine.provision` | ||
""" | ||
# self.name = kwargs.get('name', 'noname') |
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.
? :)
return {} | ||
state = STATE_MAP.get(response['cluster']['status'], config.get('CLUSTER_UNKNOWN_STATE')) | ||
|
||
key = 'cluster-{}-{}'.format(response['cluster']['name'], response['cluster']['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.
why names are used two times?
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 of them should be the id, but EKS uses the name as id at the moment, the only other option is to use the ARN which is extremely long arn:aws:eks:us-west-2:012345678910:cluster/prod
kqueen/engines/eks.py
Outdated
try: | ||
response = self.client.describe_cluster(self.cluster.metadata['id']) | ||
except Exception as e: | ||
msg = 'Fetching data from backend for cluster {} failed with the following reason:'.format(self.cluster.metadata['heat_cluster_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.
self.cluster.metadata['id']
kqueen/engines/eks.py
Outdated
}, | ||
'subnetid': { | ||
'type': 'text', | ||
'label': 'Subnet 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.
Subnet/s Id - since more than one can be chosen. Ideally we need to get list of them and provide ability to pick subnets on the fly, but for now we need to parse string to the list at least (on the UI part)
aws_secret_access_key=aws_secret_key | ||
) | ||
try: | ||
list(client.list_clusters()) |
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 we used more lightweight request here? (since this request is sending for each cluster, even for list all clusters operation)
- at least add maxResults parameter=1
- or use another request maybe generate_presigned_url?
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.
seems, No , coz afaik only list_clusters can provide valid check on all required User IAM roles/policies and etc.. I understand that its huge request, but we are locked by small api
- Template structure defined - Kubeconfig download fixed - Cluster states fixed - Engine status fixed - TODO added current issue related with kqueen-k8s-api methods: ``` api_1 | Traceback (most recent call last): api_1 | File "/code/kqueen/models.py", line 124, in status api_1 | 'addons': kubernetes.list_services(filter_addons=True), api_1 | File "/code/kqueen/kubeapi.py", line 238, in list_services api_1 | include_uninitialized=include_uninitialized api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/apis/core_v1_api.py", line 14358, in list_service_for_all_namespaces api_1 | (data) = self.list_service_for_all_namespaces_with_http_info(**kwargs) api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/apis/core_v1_api.py", line 14455, in list_service_for_all_namespaces_with_http_info api_1 | collection_formats=collection_formats) api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/api_client.py", line 321, in call_api api_1 | _return_http_data_only, collection_formats, _preload_content, _request_timeout) api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/api_client.py", line 155, in __call_api api_1 | _request_timeout=_request_timeout) api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/api_client.py", line 342, in request api_1 | headers=headers) api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/rest.py", line 231, in GET api_1 | query_params=query_params) api_1 | File "/usr/local/lib/python3.6/site-packages/kubernetes/client/rest.py", line 222, in request api_1 | raise ApiException(http_resp=r) api_1 | kubernetes.client.rest.ApiException: (403) ``` faced when try to parse k8s config body
Refactor EKS
- Typo fixes in EKS engine
Docs added
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, add new pipfile.lock
kqueen/engines/eks.py
Outdated
}, | ||
'cluster': { | ||
'node_count': { | ||
# TODO unsupported, need to define template k8s input |
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 suggest to comment node_count then
- "-i" | ||
- "<cluster-name>" | ||
# - "-r" | ||
# - "<role-arn>" |
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.
is it suppose to be commented?
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 now yes
Heptio Auth configuration
related issue kubernetes-client/python#514 |
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.
lets say I'm ok with that, but in fact I just want to hide this PR from my notifications tab :)
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.
lets say I'm ok with that, but in fact I just want to hide this PR from my notifications tab :)
current issue related with kqueen-k8s-api methods:
faced when try to parse k8s config body