Use environment var for ResolveIPAddressTimeout #2455

SmartManoj · 2024-06-30T03:35:08Z

Like #1625; Resolves #2454

Use the CUSTOM_TIMEOUT environment variable to configure the timeout for resolving IP addresses.

Import the strconv package.
Add a new variable customTimeout to read the CUSTOM_TIMEOUT environment variable.
Parse the CUSTOM_TIMEOUT value and set the timeout variable accordingly.

For more details, open the Copilot Workspace session.

SmartManoj · 2024-06-30T04:10:22Z

Will you follow this option?

How about adding a GitHub Actions workflow to sign off commits automatically if not done? This will help ensure compliance with the Developer Certificate of Origin (DCO) and streamline the contribution process.

Here is an example workflow that can be added to the repository:

name: Auto Sign-Off

on:
  pull_request:
    types: [opened, synchronize]

jobs:
  signoff:
    runs-on: ubuntu-latest

    steps:
      - name: Checkout code
        uses: actions/checkout@v2

      - name: Set up Git
        run: |
          git config --global user.name "github-actions[bot]"
          git config --global user.email "github-actions[bot]@users.noreply.github.com"

      - name: Sign off commits
        run: |
          for commit in $(git rev-list --reverse HEAD ^origin/main); do
            git cherry-pick $commit --strategy=recursive -X theirs --signoff
          done

      - name: Push changes
        run: |
          git push origin HEAD:refs/heads/${{ github.head_ref }} --force
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}

AkihiroSuda · 2024-06-30T07:05:41Z

How about adding a GitHub Actions workflow to sign off commits automatically if not done? This will help ensure compliance with the Developer Certificate of Origin (DCO) and streamline the contribution process.

Nope, rather opposite, as automated signs are unlikely valid

AkihiroSuda · 2024-06-30T07:06:16Z

pkg/networks/usernet/client.go

@@ -74,7 +74,7 @@ func (c *Client) ResolveAndForwardSSH(ipAddr string, sshPort int) error {
 }

 func (c *Client) ResolveIPAddress(ctx context.Context, vmMacAddr string) (string, error) {
-	timeout := time.After(2 * time.Minute)
+	timeout := time.After(3 * time.Minute)


Why does it take more than 2 minutes?

During #1625, Why does it take more than 1 minute?

Seems rather arbitrary, since it started out as 500ms

Seeing the same type of issues when working with mDNS - sometimes my machine is really slow to respond (like when waking up from idle) - but I also don't want to wait several seconds for when the machine really is gone. For now that (other) issue just has a timeout workaround, when doing lookups in the .local domain.

I find that the GitHub macOS runners are painfully slow, so I can see that you may need special accommodations for them. Maybe instead of relaxing timeouts generally, we should have some kind of setting, like LIMA_SLOW_HOST_FACTOR=2 that would double all the timeouts in Lima to be more forgiving?

Feels quite kludgy, but still better than making the timeouts longer for the regular use case.

@SmartManoj Can you try setting --cpus=1 --memory=1 see this helps ??

We are also using macOS runners. 2mins as timeout is working fine. I feel like even after the above 3mins change you will see some failures like this.

How will reducing memory help?

Problem with github macos runners is that if it takes more memory they will terminate the vm process / take more time to start basically slowers all other stuff.

In this memory, cpu setup we saw better success rate with macOS runners

Still the same; https://github.com/OpenDevin/OpenDevin/actions/runs/9802679379/job/27067695657

pkg/networks/usernet/client.go

jandubois

I'm fine with this change, so am approving it.

As it is kind of a hack though, I won't merge it unless it gets another approval from another @lima-vm/reviewers

afbjorklund · 2024-07-05T07:05:09Z

it is kind of a hack though, I won't merge it unless it gets another approval

It was already hack with 2 minutes, I don't see how 3 minutes make it worse...

vz: Fix usernet sshLocalPort check failure for vz driver #1625

The real hack is making GitHub Actions into a special case, is what I'm thinking

pkg/networks/usernet/client.go

SmartManoj · 2024-07-05T09:14:55Z

Why was the test failed with msg="did not receive an event with the \"running\" status"?

jandubois · 2024-07-08T18:23:10Z

Why was the test failed with msg="did not receive an event with the \"running\" status"?

We can re-run the test to see if it is just a flake, but you also need to squash your commits together and sign it with a DCO before it can be considered for merging (assuming there will otherwise be consensus that the change is desirable).

SmartManoj · 2024-07-09T07:55:52Z

Why was the test failed with msg="did not receive an event with the \"running\" status"?

Due to this timeout.

lima/pkg/start/start.go

Line 36 in 463ed82

const DefaultWatchHostAgentEventsTimeout = 10 * time.Minute

@jandubois Are there any objections to changing this in this PR?

jandubois · 2024-07-09T16:38:53Z

Due to this timeout.

lima/pkg/start/start.go

Line 36 in 463ed82

const DefaultWatchHostAgentEventsTimeout = 10 * time.Minute

@jandubois Are there any objections to changing this in this PR?

I would like to first see some evidence that a longer timeout "fixes" the issue.

I have a suspicion that something is just wedged, and it will never get ready until it is killed and restarted. In that case increasing the timeout will just mean it takes even longer before CI eventually fails.

So maybe use a much longer timeout in a branch, and re-run the tests a bunch of times, to see if they will then always pass.

Only then can we meaningfully discuss if we want to extend the timeout, and if it should again be gates by running inside GitHub actions.

SmartManoj · 2024-07-18T10:07:55Z

it will never get ready until it is killed and restarted.

first two attempts failed with msg="[hostagent] QEMU did not exit in 3m0s, forcibly killing QEMU"
third one with msg="error starting docker: cannot restart, VM not previously started" (inference => 1st essential requirement is satisfied in ~7mins (3+3+1))

here I didn't kill, stop, or delete it.

Action link

Same behavior even after stopped.

Action link

SmartManoj · 2024-07-23T07:17:22Z

Use the CUSTOM_TIMEOUT environment variable to configure the timeout for resolving IP addresses.

Import the strconv package.
Add a new variable customTimeout to read the CUSTOM_TIMEOUT environment variable.
Parse the CUSTOM_TIMEOUT value and set the timeout variable accordingly.

For more details, open the Copilot Workspace session.

pkg/networks/usernet/client.go

jandubois · 2024-08-02T01:09:44Z

@lima-vm/reviewers Where are we with this PR? I think we can merge it now, but I think the environment variable should be documented "somewhere", I just can't figure out where. Any suggestions?

SmartManoj · 2024-08-02T01:27:33Z

https://lima-vm.io/docs/usage/#customization
Here?

jandubois · 2024-08-02T01:45:28Z

https://lima-vm.io/docs/usage/#customization
Here?

I think that page is supposed to be simple and have just the high-level examples.

Maybe we need an extra page under the "Configuration Guide"?

SmartManoj · 2024-08-02T01:49:16Z

Maybe we need an extra page under the "Configuration Guide"?

With the name "Environment Variables", documenting $LIMA_INSTANCE too?

pkg/networks/usernet/client.go

AkihiroSuda · 2024-08-02T04:25:19Z

Please squash the commits

jandubois · 2024-08-02T04:27:28Z

Maybe we need an extra page under the "Configuration Guide"?

With the name "Environment Variables", documenting $LIMA_INSTANCE too?

Sounds good to me.

In that case include all the other variables too (LIMA_SHELL, LIMA_WORKDIR, LIMACTL), and document where/when they are used.

LIMA_INSTANCE and LIMACTL are supported by lima and lima.bat, but LIMA_SHELL and LIMA_WORKDIR only by lima. LIMA_INSTANCE is also supported by docker.lima and kubectl.lima... Please check all the places!

Signed-off-by: மனோஜ்குமார் பழனிச்சாமி <[email protected]>

SmartManoj · 2024-08-02T04:50:46Z

Copilot Workspace for updating docs

It touches 8 files. First 2 is enough?

Should I update the docs in this PR itself?

Created a new PR to avoid the blockage of lima-vm#2455 Add documentation for environment variables used in Lima. * **New Documentation Page**: Add `website/content/en/docs/config/environment-variables.md` to document environment variables `LIMA_INSTANCE`, `LIMA_SHELL`, `LIMA_WORKDIR`, `LIMACTL`, and `LIMA_USERNET_RESOLVE_IP_ADDRESS_TIMEOUT`. * **Update Configuration Guide**: Modify `website/content/en/docs/config/_index.md` to include a link to the new 'Environment Variables' page. * **Update README**: Modify `README.md` to include a link to the new 'Environment Variables' page. * **Add Comments in Scripts**: - `cmd/lima`: Add comments documenting `LIMA_INSTANCE`, `LIMA_SHELL`, `LIMA_WORKDIR`, and `LIMACTL`. - `cmd/lima.bat`: Add comments documenting `LIMA_INSTANCE` and `LIMACTL`. - `cmd/docker.lima`: Add comments documenting `LIMA_INSTANCE`. - `cmd/kubectl.lima`: Add comments documenting `LIMA_INSTANCE`. - `cmd/limactl/usernet.go`: Add comments documenting `LIMA_USERNET_RESOLVE_IP_ADDRESS_TIMEOUT`. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/lima-vm/lima/pull/2455?shareId=XXXX-XXXX-XXXX-XXXX).

Created a new PR to avoid the blockage of lima-vm#2455 Add documentation for environment variables used in Lima. * **New Documentation Page**: Add `website/content/en/docs/config/environment-variables.md` to document environment variables `LIMA_INSTANCE`, `LIMA_SHELL`, `LIMA_WORKDIR`, `LIMACTL`, and `LIMA_USERNET_RESOLVE_IP_ADDRESS_TIMEOUT`. * **Update Configuration Guide**: Modify `website/content/en/docs/config/_index.md` to include a link to the new 'Environment Variables' page. * **Update README**: Modify `README.md` to include a link to the new 'Environment Variables' page. * **Add Comments in Scripts**: - `cmd/lima`: Add comments documenting `LIMA_INSTANCE`, `LIMA_SHELL`, `LIMA_WORKDIR`, and `LIMACTL`. - `cmd/lima.bat`: Add comments documenting `LIMA_INSTANCE` and `LIMACTL`. - `cmd/docker.lima`: Add comments documenting `LIMA_INSTANCE`. - `cmd/kubectl.lima`: Add comments documenting `LIMA_INSTANCE`. - `cmd/limactl/usernet.go`: Add comments documenting `LIMA_USERNET_RESOLVE_IP_ADDRESS_TIMEOUT`. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/lima-vm/lima/pull/2455?shareId=XXXX-XXXX-XXXX-XXXX). Signed-off-by: மனோஜ்குமார் பழனிச்சாமி <[email protected]>

jandubois

Thanks, LGTM

jandubois · 2024-08-02T06:37:28Z

Should I update the docs in this PR itself?

No, separate PR is fine since it will document more than just the single variable in this PR.

SmartManoj · 2024-08-03T02:48:55Z

Please squash the commits

Isn't squash and merge not enabled for this repo?

jandubois · 2024-08-06T05:39:48Z

Isn't squash and merge not enabled for this repo?

No, it is up to the PR author to combine commits and write a commit message that covers the squashed commits correctly.

Also (but rarely) there is justification for squashing commits into just a smaller number of commits instead of a single one. This should only be done to separate different areas of functionality, and not to maintain the individual steps that lead to the final change.

Typically this means that the 2 separate commits should have been 2 separate PRs.

AkihiroSuda · 2024-08-08T23:09:15Z

மனோஜ்குமார் பழனிச்சாமி

Next time please consider using Latin alphabets

AkihiroSuda reviewed Jun 30, 2024

View reviewed changes

SmartManoj mentioned this pull request Jul 1, 2024

CI: For colima, modify CPU count and memory All-Hands-AI/OpenHands#2712

Merged

jandubois reviewed Jul 5, 2024

View reviewed changes

pkg/networks/usernet/client.go Outdated Show resolved Hide resolved

jandubois previously approved these changes Jul 5, 2024

View reviewed changes

jandubois requested a review from a team July 5, 2024 06:31

SmartManoj commented Jul 5, 2024

View reviewed changes

pkg/networks/usernet/client.go Outdated Show resolved Hide resolved

SmartManoj mentioned this pull request Jul 8, 2024

Error: msg="did not receive an event with the \"running\" status" #2473

Closed

SmartManoj force-pushed the patch-1 branch from 5ca1bcf to da9b66c Compare July 9, 2024 05:39

SmartManoj mentioned this pull request Jul 13, 2024

CI: Add retry mechanism All-Hands-AI/OpenHands#2915

Merged

SmartManoj mentioned this pull request Jul 22, 2024

CI: Automate colima delete command with confirmation All-Hands-AI/OpenHands#3061

Closed

SmartManoj dismissed jandubois’s stale review via f53736b July 23, 2024 07:16

SmartManoj requested review from AkihiroSuda, balajiv113, jandubois and afbjorklund July 23, 2024 07:17

alexandear reviewed Jul 23, 2024

View reviewed changes

pkg/networks/usernet/client.go Outdated Show resolved Hide resolved

pkg/networks/usernet/client.go Outdated Show resolved Hide resolved

SmartManoj force-pushed the patch-1 branch from f53736b to c46ba9d Compare July 23, 2024 10:23

SmartManoj changed the title ~~Increase timeout for resolving IP Address.~~ Use environment var for ResolveIPAddressTimeout Jul 23, 2024

SmartManoj force-pushed the patch-1 branch from ddf036b to b98a15d Compare July 23, 2024 10:30

AkihiroSuda reviewed Aug 2, 2024

View reviewed changes

pkg/networks/usernet/client.go Outdated Show resolved Hide resolved

SmartManoj force-pushed the patch-1 branch from 583344c to 400e9fd Compare August 2, 2024 04:26

Use environment var for Resolving IP Address Timeout

37fc2fd

Signed-off-by: மனோஜ்குமார் பழனிச்சாமி <[email protected]>

SmartManoj force-pushed the patch-1 branch from 400e9fd to 37fc2fd Compare August 2, 2024 04:32

SmartManoj mentioned this pull request Aug 2, 2024

Add environment variables documentation SmartManoj/lima#3

Closed

SmartManoj mentioned this pull request Aug 2, 2024

Add environment variables documentation #2526

Merged

jandubois approved these changes Aug 2, 2024

View reviewed changes

jandubois added this to the v0.23.0 milestone Aug 2, 2024

jandubois merged commit 200db6d into lima-vm:master Aug 8, 2024
26 of 27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use environment var for ResolveIPAddressTimeout #2455

Use environment var for ResolveIPAddressTimeout #2455

SmartManoj commented Jun 30, 2024 •

edited

Loading

SmartManoj commented Jun 30, 2024

AkihiroSuda commented Jun 30, 2024

AkihiroSuda Jun 30, 2024

SmartManoj Jun 30, 2024

afbjorklund Jun 30, 2024 •

edited

Loading

jandubois Jun 30, 2024 •

edited

Loading

balajiv113 Jul 1, 2024

SmartManoj Jul 1, 2024 •

edited

Loading

balajiv113 Jul 1, 2024

SmartManoj Jul 5, 2024

jandubois left a comment •

edited

Loading

afbjorklund commented Jul 5, 2024

SmartManoj commented Jul 5, 2024 •

edited

Loading

jandubois commented Jul 8, 2024

SmartManoj commented Jul 9, 2024

jandubois commented Jul 9, 2024

SmartManoj commented Jul 18, 2024 •

edited

Loading

SmartManoj commented Jul 23, 2024

jandubois commented Aug 2, 2024

SmartManoj commented Aug 2, 2024

jandubois commented Aug 2, 2024

SmartManoj commented Aug 2, 2024

AkihiroSuda commented Aug 2, 2024

jandubois commented Aug 2, 2024

SmartManoj commented Aug 2, 2024

jandubois left a comment

jandubois commented Aug 2, 2024

SmartManoj commented Aug 3, 2024

jandubois commented Aug 6, 2024

AkihiroSuda commented Aug 8, 2024

Use environment var for ResolveIPAddressTimeout #2455

Use environment var for ResolveIPAddressTimeout #2455

Conversation

SmartManoj commented Jun 30, 2024 • edited Loading

SmartManoj commented Jun 30, 2024

AkihiroSuda commented Jun 30, 2024

AkihiroSuda Jun 30, 2024

Choose a reason for hiding this comment

SmartManoj Jun 30, 2024

Choose a reason for hiding this comment

afbjorklund Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

jandubois Jun 30, 2024 • edited Loading

Choose a reason for hiding this comment

balajiv113 Jul 1, 2024

Choose a reason for hiding this comment

SmartManoj Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

balajiv113 Jul 1, 2024

Choose a reason for hiding this comment

SmartManoj Jul 5, 2024

Choose a reason for hiding this comment

jandubois left a comment • edited Loading

Choose a reason for hiding this comment

afbjorklund commented Jul 5, 2024

SmartManoj commented Jul 5, 2024 • edited Loading

jandubois commented Jul 8, 2024

SmartManoj commented Jul 9, 2024

jandubois commented Jul 9, 2024

SmartManoj commented Jul 18, 2024 • edited Loading

SmartManoj commented Jul 23, 2024

jandubois commented Aug 2, 2024

SmartManoj commented Aug 2, 2024

jandubois commented Aug 2, 2024

SmartManoj commented Aug 2, 2024

AkihiroSuda commented Aug 2, 2024

jandubois commented Aug 2, 2024

SmartManoj commented Aug 2, 2024

jandubois left a comment

Choose a reason for hiding this comment

jandubois commented Aug 2, 2024

SmartManoj commented Aug 3, 2024

jandubois commented Aug 6, 2024

AkihiroSuda commented Aug 8, 2024

SmartManoj commented Jun 30, 2024 •

edited

Loading

afbjorklund Jun 30, 2024 •

edited

Loading

jandubois Jun 30, 2024 •

edited

Loading

SmartManoj Jul 1, 2024 •

edited

Loading

jandubois left a comment •

edited

Loading

SmartManoj commented Jul 5, 2024 •

edited

Loading

SmartManoj commented Jul 18, 2024 •

edited

Loading