YNDX implemented provider for YandexCloud #2172
Conversation
|
Hi! There is some problem with CLA. @googlebot has used my personal CLA that I signed early, but for this PR we need CLA signed by Yandex LLC |
|
Thanks for the pull request! Can you provide some more details about the CLA issues you mentioned? |
|
Can someone at Yandex add you to [email protected]? |
|
@dlott, ok, I was added to [email protected], can we rerun @googlebot for using corporate CLA? |
|
@googlebot : rescan |
https://gist.github.com/elemir/5113c1473b8480055f32f5ebb304580b |
|
@dlott, hi! I'd fixed all review issues, can you review changes again? |
There was a problem hiding this comment.
Hi elemir,
Thank you for your contribution! Next steps needed to merge PR:
- Please sync PR to head
- Please add unit tests for the different resources (VM, network, etc - see other clouds as example) - this is to ensure that we do not break any YandexCloud functionality when new code is added to PKB.
- Please attach log files to the PR that show successful runs on the cluster_boot, netperf, and ping benchmarks.
- Please update functions to use pytype.
Thank you.
| 'yc_subnet_addr', '10.128.0.0/20', 'Address range to the ' | ||
| 'subnet, given in CDR notation.') | ||
| flags.DEFINE_integer( | ||
| 'yc_core_fraction', None, 'If provided, specifies baseline performance for a core in percent') |
There was a problem hiding this comment.
Lint: Line appears to be too long.
There was a problem hiding this comment.
Thanks for submitting this PR! We're always wanting to run on more clouds.
I'll sheppard your PR through getting approved. Can you run the cluster_boot and netperf benchmarks and attach the logs to the PR? That will help show us that this works as we don't have a yandex account.
| from perfkitbenchmarker import flags | ||
|
|
||
| flags.DEFINE_string('yc_path', 'yc', 'The path for the yc utility.') | ||
| flags.DEFINE_list('additional_yc_flags', [], |
There was a problem hiding this comment.
Prefix all flags for yandex with yc_ . In this case yc_flags_additional
| 'yc_subnet_zone', None, 'Zone to create subnet in ' | ||
| 'instead of automatically creating one in every zone.') | ||
| flags.DEFINE_string( | ||
| 'yc_subnet_addr', '10.128.0.0/20', 'Address range to the ' |
There was a problem hiding this comment.
Is this a required flag? For most clouds the subnet is tied to the zone and is automatically assigned. I haven't looked into YC though.
| 'yc_subnet_addr', '10.128.0.0/20', 'Address range to the ' | ||
| 'subnet, given in CDR notation.') | ||
| flags.DEFINE_integer( | ||
| 'yc_core_fraction', None, 'If provided, specifies baseline performance for a core in percent') |
There was a problem hiding this comment.
Is this percent as in "100" is 100% or is that 1.0? Include an example like "--yc_core_fraction=75 for 3/4 performance"
| from perfkitbenchmarker.providers.yandexcloud import yc_machine_types | ||
| import six | ||
|
|
||
| if six.PY2: |
There was a problem hiding this comment.
We've moved over to be python3 only so you can just use python3 constructs and not use six.
| def GetDefaultFolderId(): | ||
| """Get the default folder id.""" | ||
| conf = ParseConfig() | ||
| if "folder-id" in conf: |
There was a problem hiding this comment.
Nit: you can say return conf.get('folder-id') to return None if the key is not in there.
| self.id = None | ||
|
|
||
| if not self.cores or not self.memory: | ||
| raise errors.Config.MissingOption |
There was a problem hiding this comment.
Can you put a message in here like raise errors.Config.MissingOption('Missing cores and/or memory')
| with vm_util.NamedTemporaryFile(mode='w', dir=vm_util.GetTempDir(), | ||
| prefix='key-metadata') as tf: | ||
| tf.write(public_key) | ||
| tf.close() |
There was a problem hiding this comment.
Seems a little odd to just read in a file and spit it back out. Can you just use the self.ssh_public_key in the call to GenerateCreateCommand?
| util.CheckYcResponseKnownFailures(stderr, retcode) | ||
|
|
||
| def _CreateDependencies(self): | ||
| super(YcVirtualMachine, self)._CreateDependencies() |
There was a problem hiding this comment.
Don't need to do this unless you are going to also do something in _CreateDependencies() in your code
| 'describe', self.name) | ||
| stdout, _, _ = getinstance_cmd.Issue() | ||
| response = json.loads(stdout) | ||
| self._ParseDescribeResponse(response) |
There was a problem hiding this comment.
I usually like to log instances like this that will only happen one time and will help with debugging:
logging.info('Created VM %s with internal IP: %s, external IP: %s', self.id, self.internal_ip, self.ip_address)
| if self.preemptible: | ||
| yc_command = util.YcCommand( | ||
| 'compute', 'instance', 'list-operations', | ||
| '--filter=zone:%s targetLink.scope():%s' % (self.zone, self.name)) |
There was a problem hiding this comment.
Is this right? It looks a lot like the one for GCP
|
Hello! Is it relevant to add new providers? |
|
Yes, we can accept PRs to add other cloud providers. We typically cannot test the code so sharing a log of a simple benchmark run would be useful as well. |
Implemented provider for https://cloud.yandex.com