Include completed_at in serialized batch connect card attributes

[codecat555]

We are using the modified_at session attribute in our Connection View session cards, and we'd like to roll the change up into the official distribution.

The modified_at value is used to construct a link to our Grafana resource utilization dashboard (see below).

I have prepared a corresponding change for the ood-documentation repo, but I thought I should see how this pull request goes before submitting a pull request there.

Here's the info.html.erb we are using with this change:

<%
  grafana_url_prefix = 'YOUR-URL-PREFIX-HERE'
  start_time = created_at.to_i * 1000

  if cache_completed
    end_time = modified_at.to_i * 1000
  else
    end_time = 'now'
  end
%>
<a href="<%= grafana_url_prefix %>&from=<%= start_time %>&to=<%= end_time %>&var-jobid=<%= job_id %>&refresh=5m" target="_blank">View this job's resource utilization</a>

[johrstrom]

Hi! Thanks for the contribution. I assume modified_at is the opposite of created_at? If so, maybe we can change it to completed_at?

Beyond that - I can see tests fail. You should look the created_at field to replicate all that's needed. Basically, it's an actual attr_accessor that has to be set at some point. I'm guessing if indeed completed_at looks like you can set it in the update_cache_completed! method.

[codecat555]

Hi!

I could change the name of modified_at, but it is a pre-existing definition. It tracks the mtime of the file named self.class.db_root.join(id) session working directory.

See

ondemand/apps/dashboard/app/models/batch_connect/session.rb

Line 186 in 4ce2491

def modified_at

The surrounding methods in that file also reference modified_at and provide more clues about it's meaning/usage.

---

I looked at the created_at references, as you suggested.

I added an attr_accessor line for modified_at.

I also updated the cache_completed method to update modified_at, but I have some doubt about this change. Isn't it redundant given how modified_at is defined?

---

I want to run the tests before pushing again, but I don't know how. TBH, I didn't see the DEVELOPMENT.md doc until just now. I'll dig into that and circle back when I have some results.

[johrstrom]

I could change the name of modified_at, but it is a pre-existing definition. It tracks the mtime of the file named self.class.db_root.join(id) session working directory.

LOL I guess that's what I get for just glancing at this and not wondering if modified_at is already defined, which it obviously is (now that I actually read the file). I just glanced at this and actually didn't realize modified_at is already defined.

I also updated the cache_completed method to update modified_at, but I have some doubt about this change. Isn't it redundant given how modified_at is defined?

Yea there may be a race condition here because we're serializing a value about a file's metadata in the file itself. Consider these 2 conditions:

• the file hasn't been written yet, so we set modified_at to Time.now.to_i
• the file is about to be updated, so modified_at would actually be the previous update, not the current one, so we'd have to set it to Time.now.to_i

Maybe we should add and use completed_at just to avoid confusion.

I want to run the tests before pushing again, but I don't know how. TBH, I didn't see the DEVELOPMENT.md doc until just now. I'll dig into that and circle back when I have some results.

bundle exec rake test
# OR this to run 1 test specifically
bundle exec rake test TEST=test/models/batch_connect/session_test.rb

This is the test case that needs updating.

ondemand/apps/dashboard/test/models/batch_connect/session_test.rb

Line 533 in 4ce2491

test 'writes db file correctly' do

[codecat555]

Yes, I see your point about the race condition. I've reworked the change to define completed_at, instead.

I also updated the test case you indicated.

I ran into multiple issues trying to run the tests - I'm not really familiar with ruby, or rails, or rake or bundle. I would like to educate myself if there's a good guide available (I didn't find what I needed in DEVELOPMENT.md and elsewhere).

I'm just going to submit the change so it will at least align with our discussion so far. We'll see what happens with the tests.

Thanks very much for your help with this. The feature this enables has proved to be popular with our users.

[johrstrom]

I ran into multiple issues trying to run the tests - I'm not really familiar with ruby, or rails, or rake or bundle. I would like to educate myself if there's a good guide available (I didn't find what I needed in DEVELOPMENT.md and elsewhere).

I'm just going to submit the change so it will at least align with our discussion so far. We'll see what happens with the tests.

Here's ruby on rails docs for testing. That said, I'm happy to let the CI run the tests as well. Worst case - I can edit the tests myself to get this through, but I think that was the only test you needed to update.

https://guides.rubyonrails.org/testing.html

Thanks very much for your help with this. The feature this enables has proved to be popular with our users.

Thank you for the contribution!

[johrstrom]

🤦‍♂️ I meant to check this earlier, but forgot. The commits are not correctly tied to your github account. I was going to merge this as you have it, but I think it's important that you get the appropriate credit.

[codecat555]

Thanks for letting me know about that attribution issue. It should be fixed now, the blame credit can be assigned appropriately. :-)

I will study that documentation before submitting my next change, thanks for the link!

[codecat555]

Please note, I cleaned things up by pushing with --force so the old commit id's are no longer valid.

[johrstrom]

the blame credit can be assigned appropriately. :-)

LOL, we should be all good to go once the tests pass. I'll backport this to 3.0.x too. I don't know when that'll be released - I'm kinda waiting for other bugs to come in.

Thanks again for the contribution!

[johrstrom]

Thanks again for up-streaming this.