Draft: Fix install job creation
After changing the install job name in !137 (merged) , I noticed that the new install job was being created for already-installed sites. This is dangerous!
- Added a check on the
Initializedcondition before ensuring the install job. - Fixed the Initialized condition. It used to be set to
falseeven if the operator simply failed to exec into the pod, which is unintended. Instead, it should beUnknown.
Merge request reports
Activity
469 460 // isDrupalSiteInstalled checks if the drupal site is initialized by running drush status command in the PHP pod 470 func (r *DrupalSiteReconciler) isDrupalSiteInstalled(ctx context.Context, d *webservicesv1a1.DrupalSite) bool { 461 func (r *DrupalSiteReconciler) isDrupalSiteInstalled(ctx context.Context, d *webservicesv1a1.DrupalSite) (bool, reconcileError) { 471 462 if r.isDrupalSiteReady(ctx, d) { 472 if _, err := r.execToServerPodErrOnStderr(ctx, d, "php-fpm", nil, checkIfSiteIsInstalled()...); err != nil { 473 return false 463 _, stderr, err := r.execToServerPod(ctx, d, "php-fpm", nil, checkIfSiteIsInstalled()...) 464 // Error running exec => condition unknown 465 if err != nil { 466 return false, newApplicationError(err, ErrClientK8s) 474 467 } 475 return true 468 // The script executed and returned this error message 469 // TODO: check error code instead of message! 470 if stderr == "Drupal is not installed" { 471 return false, nil I get this, but I don't think we are not doing it right here.
When the site is being installed/ cloned, there is a good chance we run into errors. With the current logic, in these scenarios, we return
true, nilwhich is wrong. As it proceeds with the creation of resources and everything.
assigned to @ravineet
- Resolved by Konstantinos Samaras-Tsakiris
@ravineet since you are in the process of changing to the job, if you think that this logic won't be needed, we can keep this open and revert the name change of the install job. Then you take this into account in your changes to have the same high-level error handling logic. Wdyt?
- Last reply by Konstantinos Samaras-Tsakiris
mentioned in merge request !139 (merged)