add-hyperconverged-into-dependencies #11523
Conversation
DanielOsypenko
requested review from
dahorak,
yitzhak12,
fbalak,
jilju,
nehaberry,
ebondare,
amr1ta and
suchita-g
DanielOsypenko
force-pushed
the
418hyperconverged
branch
from
2872d02 to
3f5c079
Compare
pull-request-size
bot
added
size/XL
and removed
size/L
|
added mce nstallation rework into 2nd commit |
| """ | ||
| Deploy hosted OCP cluster on provisioned Provider platform | ||
|
|
||
| Args: | ||
| deploy_cnv: (bool) Deploy CNV |
There was a problem hiding this comment.
It would be good to keep some arguments documented and have **kwargs after they are listed.
There was a problem hiding this comment.
we don't use kwargs in any call, this was removed entirely, because it was bad design. Now before calling deploy_ocp we need to call deploy_dependencies, that reduces shared dependencies and makes code more flexible
There was a problem hiding this comment.
kwargs were used because deploy_ocp is overloaded method and pylint urges on missing arguments, and kinda for future use
| get_semantic_version(get_latest_release_version(), only_major_minor=True) | ||
| ) | ||
|
|
||
| if provider_ocp_version > latest_released_ocp_version: |
There was a problem hiding this comment.
This looks ok to me. If the workaround is needed for older version then we can use release-4.17 branch.
There was a problem hiding this comment.
we don't need this workaround anymore, because it was not stable at all.
Now we have option to deploy Hyperconverged from the start, in aregular way we do it will all other dependencies, instead of installing ACM -> turning off imbedded Hyperconverged -> installing Hyperconverged upstream.
So this code is redundant and not stable, hence removed
| ) | ||
|
|
||
| else: | ||
| deploy_acm_hub = False |
There was a problem hiding this comment.
Shouldn't the values be collected from config even after first deployment?
There was a problem hiding this comment.
this function is driven by ocs_ci/deployment/deployment.py:deploy_cluster which has its own deploy_cnv call.
We have it here in case we don't want to deploy Provider and use fixture, or try to deploy clients directly from other place.
So in this case it passes the calls where we do install cnv - mandatory, when config saying this and now only check that with first client cnv and rest dependencies are installed.
Hope that answers on q.
| """ | ||
| Deploy hosted OCP cluster on provisioned Provider platform | ||
|
|
||
| Args: | ||
| deploy_cnv: (bool) Deploy CNV |
There was a problem hiding this comment.
Overall LGTM, just small question about one condition.
About the Provider - Client jenkins job, should I add following two parameters also to the job UI (similarly as we have DEPLOY_ACM_HUB and DEPLOY_CNV?
* `mce_deployment` - Deploy MCE or not (Default: false)
* `deploy_hyperconverged` - Deploy hyperconverged operator or not (Default: false). Necessary for Converged clusters with hosted clients with unreleased OCP version
ocs_ci/deployment/hosted_cluster.py
Outdated
| if not config.ENV_DATA.get("deploy_acm_hub_cluster", True): | ||
| deploy_acm_hub = False |
DanielOsypenko
force-pushed
the
418hyperconverged
branch
5 times, most recently
from
f28c27c to
0cb14ef
Compare
DanielOsypenko
force-pushed
the
418hyperconverged
branch
from
0cb14ef to
d54fdc4
Compare
Signed-off-by: Daniel Osypenko <[email protected]>
Signed-off-by: Daniel Osypenko <[email protected]>
Signed-off-by: Daniel Osypenko <[email protected]>
DanielOsypenko
force-pushed
the
418hyperconverged
branch
3 times, most recently
from
e86eb10 to
f9e7164
Compare
Signed-off-by: Daniel Osypenko <[email protected]>
DanielOsypenko
force-pushed
the
418hyperconverged
branch
from
f9e7164 to
bea9bc9
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dahorak, DanielOsypenko, fbalak The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
In past we had a problem with deploying ODF on Hosted Client clusters that were deployed with latest unreleased CVN and latest unreleased ACM.
The nature of the problem was not determined, but installing upstream hyperconverged along with upstream mce proved to be sustainable.
This PR is also giving us a backdoor option, when CNV ureleased version is still not available and upstream HCO is already been populated via unstable index-tag of quay.io/kubevirt/hyperconverged-cluster-index
Link to the issue: https://issues.redhat.com/browse/OCPBUGS-32196
Link to verification run: https://privatebin.corp.redhat.com/?08e39cfadafad0c6#E5JtjD7BpRWhLi22Jobta6U3FX6hJ3RfgUJZcAvqjsUT
Added int parameter in configuration
ENV_DATA.clusters.<cluster_name>.storage_quota. Default value - no storage_quota, meaning storage for this client is unlimited