quincy: cephadm: only pull host info from applied spec, don't try to parse yaml

[adk3798]

backport tracker: https://tracker.ceph.com/issues/58454

---

backport of #48496
parent tracker: https://tracker.ceph.com/issues/57870

this backport was staged using ceph-backport.sh version 16.0.0.6848
find the latest version at https://github.com/ceph/ceph/blob/main/src/script/ceph-backport.sh

[adk3798]

https://pulpito.ceph.com/adking-2023-02-15_21:05:54-orch:cephadm-wip-adk3-testing-2023-02-15-0956-quincy-distro-default-smithi/

1 failure in test_cephadm task due to mistake in another PR that made task bootstrap with main image rather than quincy.

1 dead job in nfs-ingress test due to failure to re-image machine. Other nfs-ingress tests in the run passed okay so didn't bother with rerun.

[adk3798]

jenkins test api

[tnyeanderson]

Is there a reason the yaml parsing library isn't being used here? Seems like reinventing the wheel...

EDIT: Here because I'm getting "Unable to parse" when I pass --apply-spec to the bootstrap command, but not when I orch apply -i directly

[tnyeanderson]

@adk3798 Okay looking closely at this PR, I think it will solve my "Unable to parse" issue (that was actually due to a blank line at the end of the YAML file). I still think that the yaml parsing library should be used here, as it's already a dependency of the orchestrator module anyway. Then all of these edge cases can be avoided.

Just to further the example, this is valid YAML that would fail to parse, even with this PR:

service_type: host
hostname: |
  myhostname
addr: >
  10.0.0.10

Or even something as simple as hostname: "myhostname" doesn't seem like it would work! All of these parts of the YAML spec should be covered, and they already are with the orchestrator which actually applies it, evidenced by the fact that the actual orch apply that gets called right after ends up working perfectly.

I'll try to get a PR in to use the yaml library to solve these problems.

[rkachach]

@adk3798 Okay looking closely at this PR, I think it will solve my "Unable to parse" issue (that was actually due to a blank line at the end of the YAML file). I still think that the yaml parsing library should be used here, as it's already a dependency of the orchestrator module anyway. Then all of these edge cases can be avoided.

Just to further the example, this is valid YAML that would fail to parse, even with this PR:

service_type: host
hostname: |
  myhostname
addr: >
  10.0.0.10

Or even something as simple as hostname: "myhostname" doesn't seem like it would work! All of these parts of the YAML spec should be covered, and they already are with the orchestrator which actually applies it, evidenced by the fact that the actual orch apply that gets called right after ends up working perfectly.

I'll try to get a PR in to use the yaml library to solve these problems.

@tnyeanderson we can't use yaml library here because this change impacts the cephadm binary (not the mgr module). Unfortunately, in this case we can't use any library which is not built-in python as it's the case of yaml. There's an ongoing effort to package the cephadm binary so we can use 3rd-party libraries. In future releases that would be the case and we should be able to use yaml for parsing, meanwhile we have to use this adhoc parsing.

[tnyeanderson]

There's an ongoing effort to package the cephadm binary so we can use 3rd-party libraries. In future releases that would be the case and we should be able to use yaml for parsing, meanwhile we have to use this adhoc parsing.

Oof, that's disappointing :( but wouldn't it essentially be as straightforward as adding a requirements.txt? Looks like build.py is already set up to install those dependencies if that file is available, so anything that uses that script to "build" cephadm should work ootb. Or, more properly, a setup.py could then just read requirements.txt for install_requires (or a different pattern could be used).

Obviously you are much more familiar with the codebase than I am! For example, I have no idea how cephadm actually gets distributed/packaged on various distros, and that would be an integral part of this discussion. I just find it odd that such a critical part of ceph administration (the supported tool, all others being essentially deprecated) has such a significant limitation. And frankly, that limitation appears to have led to some complicated, unreliable, and not-so-great ways of doing things...

I'm more than happy to help get the tool "packaged" so these third-party libraries can be used. Thanks for the insight!

[adk3798]

There's an ongoing effort to package the cephadm binary so we can use 3rd-party libraries. In future releases that would be the case and we should be able to use yaml for parsing, meanwhile we have to use this adhoc parsing.

Oof, that's disappointing :( but wouldn't it essentially be as straightforward as adding a requirements.txt? Looks like build.py is already set up to install those dependencies if that file is available, so anything that uses that script to "build" cephadm should work ootb. Or, more properly, a setup.py could then just read requirements.txt for install_requires (or a different pattern could be used).

Obviously you are much more familiar with the codebase than I am! For example, I have no idea how cephadm actually gets distributed/packaged on various distros, and that would be an integral part of this discussion. I just find it odd that such a critical part of ceph administration (the supported tool, all others being essentially deprecated) has such a significant limitation. And frankly, that limitation appears to have led to some complicated, unreliable, and not-so-great ways of doing things...

I'm more than happy to help get the tool "packaged" so these third-party libraries can be used. Thanks for the insight!

@tnyeanderson We'll be looking into adding dependencies for the cephadm binary a bit later this year. The build stuff that's there now is actually fairly new, and currently only on main branch. On quincy and earlier, the cephadm binary is just a standalone python script, that was often pulled in just by curling the file from github, hence why we had to avoid having any dependencies within it. What we were hoping for is to just have the new "build" aspect for the initial reef release be extremely simple (it's literally just a single python file put through python zipapp right now) so that if we messed it up it could be easily worked around by users (they could just rename the "cephadm.py" python script to "cephadm" and it would effectively be the same as before). If there are no issues around it after the initial reef release ("built" version properly gets published on download.ceph.com, users have no problems, etc.), we were planning to start doing a lot more around it, including breaking the 10000+ line file into multiple, more manageable files, and introducing dependencies for things like this. We have an etherpad talking about it here https://pad.ceph.com/p/cephadm-refactoring

[tnyeanderson]

Makes sense, I'll follow along on the etherpad! And so glad to hear that there are plans to break up the 10k line file into more manageable chunks... Now I see why this is more complex as it seems, as is usually the case! :)

I do have a draft PR #50269 in to use the pyyaml library. Do you think I should keep it open with a note that it is dependent on the refactor, or just close it and wait for the refactor to complete?

Thanks again for the explanation!

[adk3798]

Makes sense, I'll follow along on the etherpad! And so glad to hear that there are plans to break up the 10k line file into more manageable chunks... Now I see why this is more complex as it seems, as is usually the case! :)

I do have a draft PR #50269 in to use the pyyaml library. Do you think I should keep it open with a note that it is dependent on the refactor, or just close it and wait for the refactor to complete?

Thanks again for the explanation!

I think whether you keep it open is up to you. I think if you do, you might end up with the stalebot complaining about lack of activity, but if you're willing to rebase it or something every once in a while, I personally don't care whether it's kept open or closed and remade later. The reef release is a while out (I think May or June? Don't remember off the top of my head) because it's been pushed back due to issues in our testing lab, but I've linked your PR in the etherpad to try and remember it when the time comes.

[tnyeanderson]

I'll leave it open and keep track of rebases as needed (I don't think there will be many conflicts in this area). It will be a good litmus test of the new packaging process as it becomes ready :) thanks again!