Skip to content
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

arm-runner-patch is invalid #1620

Closed
ndench opened this issue Jan 13, 2022 · 7 comments · Fixed by #1624
Closed

arm-runner-patch is invalid #1620

ndench opened this issue Jan 13, 2022 · 7 comments · Fixed by #1624

Comments

@ndench
Copy link
Contributor

ndench commented Jan 13, 2022

The arm-runner-patch.tpl is trying to apply invalid patches during boot time. By checking /var/log/user-data.log I can see the follow errors:

patching file bin/Runner.Listener.runtimeconfig.json
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file bin/Runner.Listener.runtimeconfig.json.rej
patching file bin/Runner.PluginHost.runtimeconfig.json
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file bin/Runner.PluginHost.runtimeconfig.json.rej
patching file bin/Runner.Worker.runtimeconfig.json
Hunk #1 FAILED at 8.
1 out of 1 hunk FAILED -- saving rejects to file bin/Runner.Worker.runtimeconfig.json.rej

I then checked the files that it is trying to patch and the json files do not match:

       }
     ],
     "configProperties": {
       "System.Runtime.TieredCompilation.QuickJit": true
     }
   }
 }

Instead they contain:

      }
    ],
    "configProperties": {
      "System.Reflection.Metadata.MetadataUpdater.IsSupported": false
    }
  }

Which must be making the patch fail. My runners are then failing to boot and not accepting any workflow jobs.

@ScottGuymer
Copy link
Member

I'm starting to think that this should probably not live inside this module and should be something injected in by pre or post install user data hooks.

Its likely this could change a lot over time and having to change this module each time it does seems inconvenient.

@npalm WDYT?

@npalm
Copy link
Member

npalm commented Jan 13, 2022

Agree, supporting ARM on os and architecture level by the module is fine. But maybe better to move the tailored script to an example with indeed a pre / post hook. Any help would be welcome!

@ndench
Copy link
Contributor Author

ndench commented Jan 13, 2022

I've had a go at fixing the patch in #1624, it makes the arm runners boot correctly and appear to accept a job. However the job never actually starts and eventually times out. Is this patch even required anymore?

If the patch is still necessary, I agree that it should be solved with an example in the documentation instead of being hardcoded. The user-data scripts are not as flexible as I would like as they are, so I think there's definitely some improvement that we can make there along with this change.

If someone can point me in the right direction for how to get the ARM runners working correctly again, I'm happy to do another PR for all this.

@ScottGuymer
Copy link
Member

We don't currently use ARM runners in our fleet so I'm not sure if this is needed or how to solve it I'm afraid. Maybe the original contributor could help here? Looks like it was originally added in #102 by @bdruth I wonder if its still being used.

Do you have any suggestions on how you would like to see the user data handling improved?

@ndench
Copy link
Contributor Author

ndench commented Jan 15, 2022

The userdata scripts are a little hard to customise. You can override the entire thing with userdata_template but not the other parts (ie. userdata_install_runner and userdata_start_runner). I think just giving each of them a variable that overrides them will make it much more flexible.

@npalm
Copy link
Member

npalm commented Jan 15, 2022

Agree it is quite limited. You can use the following hooks to inject the default module scripts for starting and installing the runners.

${install_runner}
${start_runner}

The injected scripts are not customisable at this moment.

@ScottGuymer
Copy link
Member

It would be possible to make the other parts customisable too. Happy to review a PR to include this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants