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

[WIP] Add notes running vmware simulator #532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Nov 27, 2023

Here are notes on running the vmware simulator

not sure if we want this in the providers or developer_setup directory

@kbrock
Copy link
Member Author

kbrock commented Nov 27, 2023

Had tried to default the historical intervals via vmware/govmomi#3191 but wasn't able to get that working.

@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2023

Checked commit kbrock@98c2777 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@kbrock
Copy link
Member Author

kbrock commented Dec 7, 2023

I put in a fix for the editing for the file, but the reset remains

vmware/govmomi#3303

@agrare
Copy link
Member

agrare commented Dec 7, 2023

@kbrock testing historical metrics collection is probably more "niche" compared with what the average developer would use this for?
What if the primary documentation is to go install github.com/vmware/govmomi/govc@latest and then vcsim -username root -password vmware ... ? Maybe we put the instructions for historical intervals in the manageiq-providers-vmware README?

# creating simulator provider

```bash
echo "127.0.0.1 vc91" > /etc/hosts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? You can just use localhost or 127.0.0.1 when adding the provider

(I know you were trying to avoid hostname collisions with >1 vcsim but that's also more advanced than the typical UI dev will need)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the end of the day, vc91 (even if in /etc/hosts) was freaking out, so I just use localhost.

# run

```bash
# I dumped to a local directory to add historical capture support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to make the doc less of a brain dump and more of a guide to others. I'm not sure what this is supposed to me as an outside reader.

@@ -0,0 +1,98 @@
# installing vmware simulator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need an introduction of some sort.

Suggested change
# installing vmware simulator
# VMware Simulator
This guide explains general usage of the VMware simulator, `vcsim`. This simulator can be used when developing the VMware provider.
## Installing VMware Simulator

Copy link
Member

@Fryguy Fryguy Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the title case change at the end too.

ln -s ~/go/bin/govc ~/bin # or /usr/local/bin
```

# creating simulator provider
Copy link
Member

@Fryguy Fryguy Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# creating simulator provider
## Generating data for a simulated provider

kill %1 # or fg ; control-c
```

# adding historicals
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking this should be another level deep under the "Running a simulator"

Suggested change
# adding historicals
### Including historical metrics data

# adding historicals

```xml
<!--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<!--
<!--


export GOVC_URL=https://root:vmware@vc91:9191/sdk GOVC_INSECURE=true
govc object.save -d my-vcenter
# stop stop temporary simulator
Copy link
Member

@Fryguy Fryguy Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# stop stop temporary simulator
# stop temporary simulator

</val>
</propSet>
```
# better editing of historicals
Copy link
Member

@Fryguy Fryguy Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this title means - feels like this is part of the previous section, so thinking it should just be removed. The TODO also make it clear of the intentions.

Suggested change
# better editing of historicals

```
# better editing of historicals

TODO: please figure out a way to add historicals without editing the files.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TODO: please figure out a way to add historicals without editing the files.
TODO: Figure out a way to add historicals without editing the files.

govc object.collect -s -dump PerformanceManager:PerfMgr perfCounter
```

# run
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# run
## Running the simulated provider from the generated data

Comment on lines +23 to +25
vcsim -username root -password vmware -app 2 -cluster 2 -dc 1 -ds 2 -folder 2 \
-host 2 -nsx 2 -pg 2 -pg-nsx 2 -pod 2 -pool 1 -standalone-host 2 -vm 2 \
-l vc91:9191 &
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why run in the backround? Seems much simpler to leave it running in the foreground and let people ctrl-c to exit than telling them to do kill %1 later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's run in the background so it can be dumped to a directory in the next step. Then it can be reused in future runs of the simulator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a few comments around this to beef up the comments/titles, which would make that clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right but that's only necessary so that you can hack in historical metrics, for normal vcsim running it isn't needed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is all moot now. vcsim out of the box properly creates those entries

Comment on lines +27 to +28
export GOVC_URL=https://root:vmware@vc91:9191/sdk GOVC_INSECURE=true
govc object.save -d my-vcenter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export GOVC_URL=https://root:vmware@vc91:9191/sdk GOVC_INSECURE=true
govc object.save -d my-vcenter
# dump simulator contents to a directory for later reuse
export GOVC_URL=https://root:vmware@vc91:9191/sdk GOVC_INSECURE=true
govc object.save -d my-vcenter

```xml
<!--
edit my-vcenter/0009-PerformanceManager-PerfMgr.xml
find nistoricalInterval
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
find nistoricalInterval
find historicalInterval

Comment on lines +23 to +25
vcsim -username root -password vmware -app 2 -cluster 2 -dc 1 -ds 2 -folder 2 \
-host 2 -nsx 2 -pg 2 -pg-nsx 2 -pod 2 -pool 1 -standalone-host 2 -vm 2 \
-l vc91:9191 &
Copy link
Member

@Fryguy Fryguy Dec 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not required, but I tend to find guides more readable if they are one param per line (at least for very long sets of options). It also allows easier modification for things like removing lines.

Suggested change
vcsim -username root -password vmware -app 2 -cluster 2 -dc 1 -ds 2 -folder 2 \
-host 2 -nsx 2 -pg 2 -pg-nsx 2 -pod 2 -pool 1 -standalone-host 2 -vm 2 \
-l vc91:9191 &
vcsim \
-username root \
-password vmware \
-app 2 \
-cluster 2 \
-dc 1 \
-ds 2 \
-folder 2 \
-host 2 \
-nsx 2 \
-pg 2 \
-pg-nsx 2 \
-pod 2 \
-pool 1 \
-standalone-host 2 \
-vm 2 \
-l vc91:9191 &

Comment on lines +13 to +14
ln -s ~/go/bin/vcsim ~/bin # or /usr/local/bin
ln -s ~/go/bin/govc ~/bin # or /usr/local/bin
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal preference, so not required for merge, but this format always messes with my head cause it makes me think the entire bin dir is going to be pointing to the target. I like it more explicit...something like:

Suggested change
ln -s ~/go/bin/vcsim ~/bin # or /usr/local/bin
ln -s ~/go/bin/govc ~/bin # or /usr/local/bin
ln -s ~/go/bin/vcsim ~/bin/vcsim # or /usr/local/bin/vcsim
ln -s ~/go/bin/govc ~/bin/govc # or /usr/local/bin/govc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to either change your GOPATH or add ~/go/bin/ to your PATH?

Comment on lines +5 to +8
git clone https://github.com/kbrock/govmomi
cd govmomi
git remote add upstream https://github.com/vmware/govmomi
git checkout miq_counters_kb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be linking a personal fork/branch in the guides? Is this a missing feature on upstream?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed. we can now use master now that this is all merged

@agrare
Copy link
Member

agrare commented Dec 7, 2023

@Fryguy @kbrock high level I'm wondering if we're trying to show devs how to run a vmware simulator, or show them how to patch in @kbrock 's metrics fix?

This all would be drastically simpler and I think cover 90% of the use cases if we just say go install ... and vcsim ... and when his fix is merged upstream then we don't need to do anything to be able to support metrics.

@Fryguy
Copy link
Member

Fryguy commented Dec 8, 2023

Oh yeah I thought this was "how to use VMware simulator" with an added "and dump to a directory that can be reused".

@Fryguy
Copy link
Member

Fryguy commented Dec 8, 2023

Actually now that you said that, I'm thinking this entire doc should probably be in the VMware repo in the Readme or in a docs dir, and then here maybe only a link to it from a "testing providers" doc or something.

@agrare
Copy link
Member

agrare commented Dec 8, 2023

#532 (comment) 😉

@Fryguy
Copy link
Member

Fryguy commented Dec 8, 2023

Oh yeah I saw that, but I read it as "leave the base one here and puts the metrics one over there", whereas I'm saying, "put the entire doc over there with the metrics too"

@kbrock
Copy link
Member Author

kbrock commented Dec 8, 2023

@kbrock kbrock changed the title Add notes running vmware simulator [WIP] Add notes running vmware simulator Feb 23, 2024
@kbrock kbrock added the wip label Feb 23, 2024
@kbrock
Copy link
Member Author

kbrock commented Feb 23, 2024

WIP:
TODO: change to drop the editing of files, dumping of entries, and using non standard git location.

@miq-bot miq-bot added the stale label May 27, 2024
@miq-bot
Copy link
Member

miq-bot commented May 27, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot
Copy link
Member

miq-bot commented Sep 9, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

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

Successfully merging this pull request may close these issues.

4 participants